From 4dfce4b8cbb6e83782f304652ff0e4d09043bacc Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Sun, 2 Aug 2026 15:38:32 +0200 Subject: [PATCH 1/2] feat(reports): filter report attachments by document tier per report type (#1930) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Documents form an ordered evidentiary tier — quotation -> deposit -> invoice — and each report type now declares a tier floor: a document is embedded iff its tier is at or above that floor. budget-overview floors at quotation (embeds everything), claim floors at deposit (never embeds a quotation), and proof-of-funds floors at invoice (invoices only). The rule depends solely on the report type and the document's own type, replacing the previous per-invoice status/deposit-split stage matching in sourceReportService.ts, which made attachment inclusion depend on invoice status rather than the report's purpose. Untagged (null) attachmentType documents are treated as the strongest tier (invoice), so existing pre-#1877 links and untyped links are never silently dropped from claim or proof-of-funds reports. Fixes #1930 Co-Authored-By: Claude dev-team-lead Co-Authored-By: Claude backend-developer Co-Authored-By: Claude qa-integration-tester Co-Authored-By: Claude product-owner Co-Authored-By: Claude product-architect --- .../product-architect/story-reviews.md | 23 +++ .claude/agent-memory/product-owner/MEMORY.md | 2 +- .../product-owner/bank-report-wizard.md | 24 ++++ .../qa-integration-tester/MEMORY.md | 1 + .../story-1930-attachment-tier.md | 34 +++++ .../shared/attachmentTierUtils.test.ts | 85 +++++++++++ .../services/shared/attachmentTierUtils.ts | 46 ++++++ .../src/services/sourceReportService.test.ts | 133 +++++++++++------- server/src/services/sourceReportService.ts | 42 +----- wiki | 2 +- 10 files changed, 306 insertions(+), 86 deletions(-) create mode 100644 .claude/agent-memory/qa-integration-tester/story-1930-attachment-tier.md create mode 100644 server/src/services/shared/attachmentTierUtils.test.ts create mode 100644 server/src/services/shared/attachmentTierUtils.ts diff --git a/.claude/agent-memory/product-architect/story-reviews.md b/.claude/agent-memory/product-architect/story-reviews.md index 1ee608091..d9276da6b 100644 --- a/.claude/agent-memory/product-architect/story-reviews.md +++ b/.claude/agent-memory/product-architect/story-reviews.md @@ -319,3 +319,26 @@ retired the vendor concern outright and isolated the two genuinely uncapped chan Downgrade discipline: the glyph-advance finding went HIGH (round 2) -> MEDIUM (round 3) **because the structural fix changed its blast radius**, not because the numbers improved. Re-derive severity from the current architecture, not from the previous round's ranking. + +### Round 4 (APPROVED, 2026-08-02) + +Cell-scope fix verified by rendering: all three round-3 drop scenarios closed (665.8/691.0/1119.4pt +-> 404/404/264pt), page counts now grow instead of saturating, width still exactly 515.28pt across +20 cases. Caps 650/450 re-measured at a glyph 13% wider than the team used — still 13.3%/27.0% margin. + +Fourth channel found as asked: **`markerText`** (one `*N` per skipped document, unbounded, no chunk, +no break-all) — break-even ~250 skipped docs on one invoice. Not blocking; noted as follow-up. + +Two review lessons worth keeping: + +1. **Severity must be re-derived from the current architecture each round, not carried forward.** The + glyph-advance finding went HIGH -> MEDIUM -> non-blocking across rounds 2/3/4 while the *numbers + got worse* (0.89 -> 1.04 claimed, 1.18 actual). What changed was blast radius: once the `'*'` + column died, under-flagging could only paint outside a cell. Ranking a finding by its measured + error rather than its consequence would have blocked a correct PR. +2. **A comment that overclaims is its own recurring defect.** "Safely above every character scanned" + was wrong at 0.89 and again at 1.04. The durable fix is to make the bound *name its own scope* + ("widest in the Latin/German/punctuation set scanned") rather than to keep raising the number. + +Four rounds total. Trajectory was right each time; each round bounded something real and revealed the +next layer. Worth remembering before pushing for a five-round rewrite: the arc converged. diff --git a/.claude/agent-memory/product-owner/MEMORY.md b/.claude/agent-memory/product-owner/MEMORY.md index f498b9125..56a666d8f 100644 --- a/.claude/agent-memory/product-owner/MEMORY.md +++ b/.claude/agent-memory/product-owner/MEMORY.md @@ -37,7 +37,7 @@ Full detail in [standalone-bugs-and-stories.md](standalone-bugs-and-stories.md) - Auto-itemize: #1545/#1546/#1547 mini-epic (2026-05-21), #1600 (2026-05-26), **#1833 duplicate budget lines on commit retry (2026-07-07)** - Diary: #1426 critical photo data loss (2026-05-15) - Photo: #1723 lightbox picker UX (2026-06-16) -- **Bank Report Wizard mini-epic** (no parent epic): #1876 refunds (PR #1880) → #1877 contact/household/attachment typing (PR #1883) → #1878 report backend → #1879 wizard+PDF (PR #1887, CHANGES_REQUIRED then **APPROVED** round 2, 2026-07-30). Domain decisions, contract facts (refund sign, `isSplit`, `totalAmount` scope) and deferrals in [bank-report-wizard.md](bank-report-wizard.md). **Refinement Round 2** (2026-07-31, no parent epic, all Todo): #1898 report table refinements (PR #1902, merged) → #1899 settings step + report language (PR #1903, PO review 2026-07-31: **APPROVED w/ 1 MUST FIX** — 5-step wizard, `getFixedT`/`createFormatters` threading and en/de report output all verified; AC 2.2 default-locale seeding is stale on hard load, see [pr-review-patterns.md](pr-review-patterns.md) `useState(contextValue)` entry) → #1900 editable HTML preview (PR #1909, round 1 COMMENT + 4 MUST FIX, **round 2 2026-07-31 APPROVED** — all 4 verified on disk: local `composes` classes, `--font-weight-medium`, `sharedStyles.srOnly`, translated `resetFieldAriaLabel` at all 9 sites w/ en+de parity; stylelint exit 0. Note: `gh pr review --approve` fails when PO authored the PR context — post verdict via `gh pr comment` with explicit Verdict line. Judgment rulings: signature-derived-from-sender ACCEPTED, mark-claimed-generates-no-PDF ACCEPTED as vacuous, per-field reset ACCEPTED, AC 4.6 rendered-preview assertion ACCEPTED as documented deviation — Playwright headless has no PDF viewer plugin, so the E2E asserts the CSP `frame-src` contract instead; **mixed-language mobile cards ACCEPTED** — see [bank-report-wizard.md](bank-report-wizard.md) "artifact content vs. edit affordance") → #1901 AI usage/cover-letter generation (PR #1916, PO review 2026-07-31: **CHANGES_REQUIRED** — 3 blocking numeric-accuracy defects in the LLM prompt inputs: `/100` on major-unit amounts, `Math.round` to whole euros, per-invoice amount ignoring `excludedLineIds`; + 2 MUST FIX: extraction-flavoured shared LLM error copy, uncommitted wiki API-Contract section. All 6 AC sections otherwise met; entity-level linked-item description deviation ACCEPTED. New defect class recorded in [pr-review-patterns.md](pr-review-patterns.md) "LLM/prompt-assembly defects". **Round 2 on `b70d821b`: APPROVED** — all 5 findings fixed and verified on disk; `prompts.test.ts` gained a dedicated ×100 regression-guard block (98/98 pass locally); per-invoice cents-rounding now makes server math identical to client `applyLineExclusions`; wiki pushed at `254db1d`; the 9 removed test lines were a stale #1915 header note, not a weakened assertion). **Follow-ups consolidated into #1917** (tech-debt, Should Have, Backlog): architect M1–M4 + L1/L2/L3/L5, the `Konstruktionsprojekt`→`Bauprojekt` prompt nit, and the approved `KI` glossary entry. M2 (extract `computeIncludedTotal` to `@cornerstone/shared`) is the headline — the client/server duplication already drifted once and caused the #1916 blocking bug. Open: **#1891** user-verification follow-up (Todo, PR #1894 **APPROVED** 32/32 round 2, 2026-07-30 — 2 wiki MUST FIX outstanding); **#1888** stage-matched attachment indicator (Backlog, blocked-by #1879); **#1895** HIGH claim close-out cross-source sweep, **#1896** quotation-deposit 409 (blocked-by #1895), **#1897** deposit-blind drill-down — all Backlog, from the #1891 architect audit; **#1910** `lang` attribute on report-language preview content (Backlog, a11y follow-up from #1909 round 2); E2E shard 5 pre-existing flake must be triaged before promoting to `main`. **Refinement Round 3** (2026-08-02, from user PDF inspection + wizard walkthrough, all Todo, for `/batch-develop`): **#1929** PDF layout robustness (bug, Must Have — column widths, `dontBreakRows`, header clipped by 40pt top margin; **PR #1935 CHANGES_REQUIRED ×2, AC2-vs-AC4 conflict ruled 2026-08-02: precedence ladder I1 no-loss > I2 no-clip > I3 row-whole > I4 no-word-break; AC2/3/4 rewritten, AC12–AC14 added; 600-char target**), **#1930** attachment tier rules per report type (quotation→deposit→invoice; null = tier `invoice`; supersedes #1888's design question), **#1931** single "Enhance with AI" button + purpose-focused prompt (takes the `Konstruktionsprojekt` nit off #1917), **#1932** cover letter overhaul (folds in #1925, reverses #1909's derived-signature acceptance), **#1933** Select Invoices step UI fixes. Rulings in [bank-report-wizard.md](bank-report-wizard.md) §"Refinement Round 3". +- **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), **#1931** single "Enhance with AI" button + purpose-focused prompt (takes the `Konstruktionsprojekt` nit off #1917), **#1932** cover letter overhaul (folds in #1925, reverses #1909's derived-signature acceptance), **#1933** Select Invoices step UI fixes. Rulings in [bank-report-wizard.md](bank-report-wizard.md) §"Refinement Round 3". **#1929 CLOSED 2026-08-02** — PR #1935 merged (squash `1c5aa62c`) after **4 rounds**; both reviewers measured by real render+rasterize. 5 follow-ups filed: **#1937** German header labels break mid-word (bug, Todo, translator fast-follow — widening measured and rejected), **#1938** running-header `generated at` label with no timestamp on pages 2+ (bug, Todo, **pre-existing**), **#1939** reportPdf geometry hygiene (tech-debt, Todo, **blocks #1932** — `HEADER_ROW_HEIGHT`→`_MAX` 68pt vs measured 45.81pt, char-advance comment scoping, `PDF_STYLES` relocation), **#1940** continuation rows read as broken (could have, Backlog), **#1941** override fields have no `maxLength` (could have, Backlog). `markerText`+`invoiceNumber` folded into #1939 as documentation-only; vendor-name mid-word break recorded as accepted limitation in #1937. Detail in [bank-report-wizard.md](bank-report-wizard.md) §"#1929 closed". ## Requirements Coverage diff --git a/.claude/agent-memory/product-owner/bank-report-wizard.md b/.claude/agent-memory/product-owner/bank-report-wizard.md index 1eb85ad6b..09b976763 100644 --- a/.claude/agent-memory/product-owner/bank-report-wizard.md +++ b/.claude/agent-memory/product-owner/bank-report-wizard.md @@ -183,3 +183,27 @@ PR #1935 got CHANGES_REQUIRED from both `product-architect` and `ux-designer`. A - **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. - **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. + +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. +- **#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. + +### 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. +- **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. diff --git a/.claude/agent-memory/qa-integration-tester/MEMORY.md b/.claude/agent-memory/qa-integration-tester/MEMORY.md index 20648880b..46182e4f0 100644 --- a/.claude/agent-memory/qa-integration-tester/MEMORY.md +++ b/.claude/agent-memory/qa-integration-tester/MEMORY.md @@ -15,6 +15,7 @@ ## Recent bug/story notes (2026-08) +- [Story #1930 — attachment tier rules](story-1930-attachment-tier.md) (2026-08-02) — new `attachmentTierUtils.test.ts` (100% cov, table+explicit-null pattern); sourceReportService.test.ts 16a/b/c/e → AC1/AC2/AC3/AC5; cross-report-type test needs a fresh invoice per report type (no single status is in all 3 target slices). - [Issue #1929 round 2 — real-render pdfmake technique](story-1929-round2-real-render-technique.md) (2026-08-02, PR #1935) — `_calcWidth`/`.positions[].pageNumber` are readable after a real `getBlob()` on a held content reference (round-1's "inaccessible" comment was wrong, verified against pdfmake@0.3.11 source); measured Usage column widths (138.28pt/186.78pt, both locales, both shapes); git-stash/backup technique to prove tests genuinely fail on unpatched code; tighter layouts break old page-count fixture assumptions (scale fixtures up, don't weaken assertions); `expect(async () => {}).not.toThrow()` is a no-op anti-pattern. - [Story #1923 — report table cleanup](story-1923-report-table-cleanup.md) (2026-08-02) — unnumbered shared †/‡ markers, isDeposit/isClaim/areaText fixture ripple across 8 report test files, overviewPdf allocated-cell-is-always-an-array-of-runs gotcha, worktree `node_modules/@cornerstone/shared` symlink pointing at a differently-branched base repo (false-positive `tsc` errors; trust jest). - [Bugs #1895/#1896/#1918 — claim/deposit scope fixes](bugs-1895-1896-1918-claim-deposit-scope.md) (2026-08-01) — `markInvoicesClaimed` gained `sourceId`+required `depositIds` params (cross-source claim guard, decoupled sweep, quotation+sweepable-deposit no longer 409s); `getSourceReport` drops zero-portion `budgetLines[]` on `claim` reports only; text-content query collision gotcha (banner text contains data also shown elsewhere on page). diff --git a/.claude/agent-memory/qa-integration-tester/story-1930-attachment-tier.md b/.claude/agent-memory/qa-integration-tester/story-1930-attachment-tier.md new file mode 100644 index 000000000..d2c9901a5 --- /dev/null +++ b/.claude/agent-memory/qa-integration-tester/story-1930-attachment-tier.md @@ -0,0 +1,34 @@ +--- +name: story-1930-attachment-tier +description: Attachment tier rules per report type (quotation->deposit->invoice) replace stage matching — new pure-util test file + sourceReportService.test.ts scenario replacement +metadata: + type: project +--- + +Story #1930 (2026-08-02, branch feat/1930-attachment-tier-rules): backend-developer replaced +per-invoice stage matching in `sourceReportService.ts` step h with a report-type tier floor, +extracted to `server/src/services/shared/attachmentTierUtils.ts` (`ATTACHMENT_TIER`, +`REPORT_TYPE_TIER_FLOOR`, `isDocumentIncludedForReportType`). + +**Test files**: new `attachmentTierUtils.test.ts` (14 tests, 100% coverage, table-driven 9-case +AC1 grid + 3 explicitly-named null-tier tests per AC4) and `sourceReportService.test.ts` scenarios +16a/16b/16c/16e (stale, asserted the removed stage-derivation) replaced with 4 new tests: scenario +16 (AC1 table-driven, 3 report types x 1 invoice-with-all-doc-types each), AC2 (quotation never in +claim), AC3 (deposit-only never in proof-of-funds), AC5 (status-independence — quotation-status vs +paid-status invoice filter identically). Kept 16d (documentId passthrough) and 17/18 (Paperless +ASN/title) verbatim — unaffected. Full file: 66 tests pass, sourceReportService.ts 100% +stmts/100% funcs/100% lines (93.18% branch — pre-existing gaps unrelated to this change, e.g. +Rail-B invoice-lookup null guards at L233-237). + +**Gotcha for AC1 table-driven cross-report-type test**: the 3 report types have non-overlapping +target-status slices in places (`budget-overview` = all 4 statuses, `claim` = {pending,paid}, +`proof-of-funds` = {claimed} only) — you cannot reuse one single invoice across all 3 +`getSourceReport()` calls in one test, since no single status is in all 3 slices simultaneously +(e.g. 'paid' is in budget-overview+claim but not proof-of-funds; 'claimed' is in +budget-overview+proof-of-funds but not claim). Solution: a small helper that builds a fresh +source+invoice+all-4-doc-types combo per block, with a status valid for that block's report type, +scoped so `result.invoices` has exactly 1 entry (no need to filter by invoiceId). + +AC6 regression guard (scenarios 8-15b: status-slice selection, isSplit, refund-adjustment, +zero-drop, unallocated) untouched and confirmed still green — no evidence the change leaked +beyond `documents[]` filtering. diff --git a/server/src/services/shared/attachmentTierUtils.test.ts b/server/src/services/shared/attachmentTierUtils.test.ts new file mode 100644 index 000000000..72be796dd --- /dev/null +++ b/server/src/services/shared/attachmentTierUtils.test.ts @@ -0,0 +1,85 @@ +/** + * Unit tests for attachmentTierUtils.ts + * + * Story #1930 — Attachment tier rules per report type (quotation -> deposit -> invoice) + * replace per-invoice stage matching. + * + * Covers: the tier/floor constant shapes (locking the ordering independent of the + * function under test) and isDocumentIncludedForReportType()'s inclusion decision for + * every (report type x attachmentType) combination, including the null-handling ruling + * (AC4) which gets its own explicitly named tests since it is the rule most likely to + * be silently broken by a future refactor. + */ + +import { describe, it, expect } from '@jest/globals'; +import type { AttachmentType, SourceReportType } from '@cornerstone/shared'; +import { + ATTACHMENT_TIER, + REPORT_TYPE_TIER_FLOOR, + isDocumentIncludedForReportType, +} from './attachmentTierUtils.js'; + +// ─── Constants ────────────────────────────────────────────────────────────── + +describe('ATTACHMENT_TIER', () => { + it('is exactly { quotation: 1, deposit: 2, invoice: 3 }', () => { + expect(ATTACHMENT_TIER).toEqual({ quotation: 1, deposit: 2, invoice: 3 }); + }); +}); + +describe('REPORT_TYPE_TIER_FLOOR', () => { + it("is exactly { 'budget-overview': 1, claim: 2, 'proof-of-funds': 3 }", () => { + expect(REPORT_TYPE_TIER_FLOOR).toEqual({ + 'budget-overview': 1, + claim: 2, + 'proof-of-funds': 3, + }); + }); +}); + +// ─── isDocumentIncludedForReportType ─────────────────────────────────────── + +describe('isDocumentIncludedForReportType', () => { + // AC1: 3 report types x 3 typed attachmentType values (9 cases), per the tier-floor table. + const cases: Array<{ + reportType: SourceReportType; + attachmentType: AttachmentType; + expected: boolean; + }> = [ + // budget-overview: floor = quotation (1) -> embeds quotation, deposit, invoice + { reportType: 'budget-overview', attachmentType: 'quotation', expected: true }, + { reportType: 'budget-overview', attachmentType: 'deposit', expected: true }, + { reportType: 'budget-overview', attachmentType: 'invoice', expected: true }, + // claim: floor = deposit (2) -> embeds deposit, invoice; never quotation + { reportType: 'claim', attachmentType: 'quotation', expected: false }, + { reportType: 'claim', attachmentType: 'deposit', expected: true }, + { reportType: 'claim', attachmentType: 'invoice', expected: true }, + // proof-of-funds: floor = invoice (3) -> embeds invoice only + { reportType: 'proof-of-funds', attachmentType: 'quotation', expected: false }, + { reportType: 'proof-of-funds', attachmentType: 'deposit', expected: false }, + { reportType: 'proof-of-funds', attachmentType: 'invoice', expected: true }, + ]; + + it.each(cases)( + '$reportType + $attachmentType -> $expected', + ({ reportType, attachmentType, expected }) => { + expect(isDocumentIncludedForReportType(reportType, attachmentType)).toBe(expected); + }, + ); + + // AC4: null attachmentType is treated as tier `invoice` (the strongest tier), so it is + // included in every report type. Named individually per spec — do not fold into the + // table above, since this is the ruling most likely to be silently broken by a future + // refactor (e.g. someone "simplifying" the null branch to tier `quotation`). + it('null attachmentType is included in a budget-overview report', () => { + expect(isDocumentIncludedForReportType('budget-overview', null)).toBe(true); + }); + + it('null attachmentType is included in a claim report', () => { + expect(isDocumentIncludedForReportType('claim', null)).toBe(true); + }); + + it('null attachmentType is included in a proof-of-funds report', () => { + expect(isDocumentIncludedForReportType('proof-of-funds', null)).toBe(true); + }); +}); diff --git a/server/src/services/shared/attachmentTierUtils.ts b/server/src/services/shared/attachmentTierUtils.ts new file mode 100644 index 000000000..fcbf0ee98 --- /dev/null +++ b/server/src/services/shared/attachmentTierUtils.ts @@ -0,0 +1,46 @@ +import type { AttachmentType, SourceReportType } from '@cornerstone/shared'; + +/** + * Attachment tier model (Story #1930). + * + * Documents linked to an invoice form an ordered evidentiary tier, and each source + * report type declares a tier floor. A document is embedded in a report iff its tier + * is at or above that report type's floor. This is the ONLY place either the tier + * ordering or the per-report-type floor is defined — do not hard-code a report + * type's floor at a second site (AC9). The rule depends only on the report type and + * the document's own `attachmentType`; it must never consult invoice status, the + * deposit split, or `targetStatuses`. + */ +export const ATTACHMENT_TIER: Record = { + quotation: 1, + deposit: 2, + invoice: 3, +}; + +/** Tier floor per report type: a document must be at or above this tier to be embedded. */ +export const REPORT_TYPE_TIER_FLOOR: Record = { + 'budget-overview': ATTACHMENT_TIER.quotation, + claim: ATTACHMENT_TIER.deposit, + 'proof-of-funds': ATTACHMENT_TIER.invoice, +}; + +/** + * True iff a document with the given `attachmentType` should be embedded in a report + * of the given `reportType`. + * + * `attachmentType: null` (untagged/legacy) is treated as tier `invoice` — the + * strongest tier — so a null-typed document is included in every report type. This + * is a deliberate ruling, not an oversight: null links are legacy/ambiguous data + * (pre-#1877 links, or the invoice detail page's "Add Document" picker when the user + * skipped the type choice), not known-weak evidence. Treating null as the weakest + * tier would silently drop attachments from claim/proof-of-funds reports for + * existing data — worse than being over-inclusive, since the user can deselect an + * over-included document but cannot recover a silently-dropped one. + */ +export function isDocumentIncludedForReportType( + reportType: SourceReportType, + attachmentType: AttachmentType | null, +): boolean { + const tier = attachmentType === null ? ATTACHMENT_TIER.invoice : ATTACHMENT_TIER[attachmentType]; + return tier >= REPORT_TYPE_TIER_FLOOR[reportType]; +} diff --git a/server/src/services/sourceReportService.test.ts b/server/src/services/sourceReportService.test.ts index 8a8d4ae99..79f03e36a 100644 --- a/server/src/services/sourceReportService.test.ts +++ b/server/src/services/sourceReportService.test.ts @@ -5,9 +5,9 @@ * * Covers: * - getSourceReport: status-slice selection per report type, split-invoice detection, - * drop-on-zero / refund-adjustment classification, document stage tagging, Paperless - * ASN/title resolution (reachable / throws / unconfigured), unallocated invoices, - * totalAmount rounding. + * drop-on-zero / refund-adjustment classification, attachment-tier document filtering + * (Story #1930), Paperless ASN/title resolution (reachable / throws / unconfigured), + * unallocated invoices, totalAmount rounding. * - markInvoicesClaimed: transactional batch claim of invoices + deposits, claimability * rules, 409 rollback-on-any-offending, diary event side effects. */ @@ -498,73 +498,110 @@ describe('sourceReportService', () => { expect(result.unallocatedInvoices).toHaveLength(0); }); - it('scenario 16a: quotation-status invoice tags only the quotation document stage', async () => { - const sourceId = insertSource(); - const vendorId = insertVendor(); - const budgetId = insertWorkItemBudget(sourceId); - const invId = insertInvoice(vendorId, { status: 'quotation', amount: 500 }); - insertInvoiceBudgetLine(invId, budgetId, 500); - insertDocumentLink(invId, 1, 'quotation'); - insertDocumentLink(invId, 2, 'invoice'); - insertDocumentLink(invId, 3, 'deposit'); - insertDocumentLink(invId, 4, null); // untagged always kept + // Story #1930: per-invoice stage matching (driven by invoice status / deposit split / + // targetStatuses) was replaced by a report-type tier floor that depends only on report + // type and the document's own attachmentType. Scenarios 16a/16b/16c/16e above asserted + // the removed stage-derivation internals and are replaced by the AC1/AC2/AC3/AC5 + // scenarios below. 16d (documentId passthrough) is unaffected and kept verbatim. - const result = await getSourceReport(db, 'budget-overview', sourceId, PAPERLESS_DISABLED); - expect(result.invoices).toHaveLength(1); - const ids = result.invoices[0]!.documents.map((d) => d.attachmentType); - expect(ids.sort()).toEqual([null, 'quotation'].sort()); + it('scenario 16 (AC1): tier floor determines exactly which documents survive, per report type', async () => { + function makeInvoiceWithAllDocTypes(status: 'paid' | 'claimed'): string { + const sourceId = insertSource(); + const vendorId = insertVendor(); + const budgetId = insertWorkItemBudget(sourceId); + const invId = insertInvoice(vendorId, { status, amount: 500 }); + insertInvoiceBudgetLine(invId, budgetId, 500); + insertDocumentLink(invId, 1, 'quotation'); + insertDocumentLink(invId, 2, 'deposit'); + insertDocumentLink(invId, 3, 'invoice'); + insertDocumentLink(invId, 4, null); + return sourceId; + } + + // budget-overview: floor = quotation (1) -> quotation, deposit, invoice, null all survive. + { + const sourceId = makeInvoiceWithAllDocTypes('paid'); + const result = await getSourceReport(db, 'budget-overview', sourceId, PAPERLESS_DISABLED); + expect(result.invoices).toHaveLength(1); + const tags = result.invoices[0]!.documents.map((d) => d.attachmentType); + expect(tags.sort()).toEqual([null, 'deposit', 'invoice', 'quotation'].sort()); + } + + // claim: floor = deposit (2) -> quotation excluded; deposit, invoice, null survive. + { + const sourceId = makeInvoiceWithAllDocTypes('paid'); + const result = await getSourceReport(db, 'claim', sourceId, PAPERLESS_DISABLED); + expect(result.invoices).toHaveLength(1); + const tags = result.invoices[0]!.documents.map((d) => d.attachmentType); + expect(tags.sort()).toEqual([null, 'deposit', 'invoice'].sort()); + } + + // proof-of-funds: floor = invoice (3) -> only invoice and null survive. + { + const sourceId = makeInvoiceWithAllDocTypes('claimed'); + const result = await getSourceReport(db, 'proof-of-funds', sourceId, PAPERLESS_DISABLED); + expect(result.invoices).toHaveLength(1); + const tags = result.invoices[0]!.documents.map((d) => d.attachmentType); + expect(tags.sort()).toEqual([null, 'invoice'].sort()); + } }); - it('scenario 16b: paid-status invoice (no deposits) tags the invoice document stage', async () => { + it('AC2: a quotation-typed document never appears in a claim report', async () => { const sourceId = insertSource(); const vendorId = insertVendor(); const budgetId = insertWorkItemBudget(sourceId); + // 'paid' is squarely inside the claim report's target slice ({pending, paid}). const invId = insertInvoice(vendorId, { status: 'paid', amount: 500 }); insertInvoiceBudgetLine(invId, budgetId, 500); insertDocumentLink(invId, 1, 'quotation'); - insertDocumentLink(invId, 2, 'invoice'); - insertDocumentLink(invId, 3, 'deposit'); - insertDocumentLink(invId, 4, null); const result = await getSourceReport(db, 'claim', sourceId, PAPERLESS_DISABLED); - const tags = result.invoices[0]!.documents.map((d) => d.attachmentType); - expect(tags.sort()).toEqual([null, 'invoice'].sort()); + expect(result.invoices).toHaveLength(1); + expect(result.invoices[0]!.documents).toEqual([]); }); - it('scenario 16c: deposit whose status is in the slice tags the deposit document stage', async () => { + it('AC3: a deposit-typed document with no invoice-typed document never appears in a proof-of-funds report', async () => { const sourceId = insertSource(); const vendorId = insertVendor(); const budgetId = insertWorkItemBudget(sourceId); - const invId = insertInvoice(vendorId, { status: 'pending', amount: 1000 }); - insertInvoiceBudgetLine(invId, budgetId, 1000); - insertDeposit(invId, { amount: 300, status: 'paid', entryType: 'deposit' }); - insertDocumentLink(invId, 1, 'quotation'); - insertDocumentLink(invId, 2, 'invoice'); - insertDocumentLink(invId, 3, 'deposit'); - insertDocumentLink(invId, 4, null); + // 'claimed' is the proof-of-funds report's target slice. + const invId = insertInvoice(vendorId, { status: 'claimed', amount: 500 }); + insertInvoiceBudgetLine(invId, budgetId, 500); + insertDocumentLink(invId, 1, 'deposit'); - const result = await getSourceReport(db, 'claim', sourceId, PAPERLESS_DISABLED); // claim = {pending,paid} - const tags = result.invoices[0]!.documents.map((d) => d.attachmentType); - // residual (pending, 700/1000) tags 'invoice'; deposit (paid, 300/1000) tags 'deposit'; untagged always kept - expect(tags.sort()).toEqual([null, 'deposit', 'invoice'].sort()); + const result = await getSourceReport(db, 'proof-of-funds', sourceId, PAPERLESS_DISABLED); + expect(result.invoices).toHaveLength(1); + expect(result.invoices[0]!.documents).toEqual([]); }); - it('scenario 16e: deposit whose status is outside the slice does not tag the deposit stage', async () => { + it('AC5: invoice status is irrelevant to document selection — quotation-status and paid-status invoices filter identically', async () => { const sourceId = insertSource(); const vendorId = insertVendor(); - const budgetId = insertWorkItemBudget(sourceId); - const invId = insertInvoice(vendorId, { status: 'pending', amount: 1000 }); - insertInvoiceBudgetLine(invId, budgetId, 1000); - // 'claimed' is not part of the 'claim' report's target statuses ({pending, paid}). - insertDeposit(invId, { amount: 100, status: 'claimed', entryType: 'deposit' }); - insertDocumentLink(invId, 1, 'deposit'); - insertDocumentLink(invId, 2, null); - const result = await getSourceReport(db, 'claim', sourceId, PAPERLESS_DISABLED); - const tags = result.invoices[0]!.documents.map((d) => d.attachmentType); - // Deposit stage is never activated (its only status is out-of-slice), so the - // 'deposit'-tagged link is filtered out — only the untagged link survives. - expect(tags).toEqual([null]); + const budgetA = insertWorkItemBudget(sourceId); + const invA = insertInvoice(vendorId, { status: 'quotation', amount: 300 }); + insertInvoiceBudgetLine(invA, budgetA, 300); + insertDocumentLink(invA, 1, 'deposit'); + + const budgetB = insertWorkItemBudget(sourceId); + const invB = insertInvoice(vendorId, { status: 'paid', amount: 400 }); + insertInvoiceBudgetLine(invB, budgetB, 400); + insertDocumentLink(invB, 2, 'deposit'); + + // Both statuses fall within the budget-overview slice, so both invoices are present. + const result = await getSourceReport(db, 'budget-overview', sourceId, PAPERLESS_DISABLED); + expect(result.invoices).toHaveLength(2); + + const docsA = result.invoices + .find((i) => i.invoiceId === invA)! + .documents.map((d) => d.attachmentType); + const docsB = result.invoices + .find((i) => i.invoiceId === invB)! + .documents.map((d) => d.attachmentType); + + // Identically-typed documents survive identically, regardless of invoice status. + expect(docsA).toEqual(['deposit']); + expect(docsB).toEqual(['deposit']); }); it('scenario 16d: surviving document objects carry the real Paperless document id', async () => { diff --git a/server/src/services/sourceReportService.ts b/server/src/services/sourceReportService.ts index 13eb2c232..687a9f70c 100644 --- a/server/src/services/sourceReportService.ts +++ b/server/src/services/sourceReportService.ts @@ -21,10 +21,10 @@ import { NotFoundError, ValidationError, InvoicesNotClaimableError } from '../er import { toCents } from './shared/money.js'; import { computeLineContributionsExcludingTagged, - splitByDepositsExcludingTagged, sumTaggedDepositContributionsByInvoice, type DepositAwareRow, } from './shared/depositAggregateUtils.js'; +import { isDocumentIncludedForReportType } from './shared/attachmentTierUtils.js'; import { onInvoiceStatusChanged, onDepositStatusChanged } from './diaryAutoEventService.js'; import * as paperlessService from './paperlessService.js'; import { ALLOWED_TRANSITIONS } from './invoiceDepositService.js'; @@ -294,10 +294,7 @@ export async function getSourceReport( isSplitMap.set(row.invoice_id, row.source_count > 1); } - // Step g: Compute deposit splits excluding tagged (for stage-determination logic) - const splitsByInvoiceId = splitByDepositsExcludingTagged(railARows); - - // Step h: Batch fetch document links + // Step g: Batch fetch document links const allInvoiceIds = Array.from(invoiceMetadata.keys()); const allDocumentLinks = db .select() @@ -311,38 +308,11 @@ export async function getSourceReport( const allPaperlessDocIds = new Set(); for (const invoiceId of allInvoiceIds) { - const split = splitsByInvoiceId.get(invoiceId); - - // Determine stages: if deposit-only (no split entry), include 'deposit' if any tagged deposit in slice - const stages = new Set(); - if (split) { - if (split.invoiceStatus === 'quotation' && targetStatuses.has('quotation')) { - stages.add('quotation'); - } - if ( - split.residualFraction > 0 && - split.invoiceStatus !== 'quotation' && - targetStatuses.has(split.invoiceStatus) - ) { - stages.add('invoice'); - } - for (const df of split.depositFractions) { - if (targetStatuses.has(df.depositStatus)) { - stages.add('deposit'); - } - } - } else { - // Deposit-only: check if any tagged deposit in target statuses - if (railBContributions.has(invoiceId)) { - stages.add('deposit'); - } - } - const invoiceLinks = allDocumentLinks .filter( (link) => link.entityId === invoiceId && - (link.attachmentType === null || stages.has(link.attachmentType as AttachmentType)), + isDocumentIncludedForReportType(type, link.attachmentType as AttachmentType | null), ) .map((link) => { allPaperlessDocIds.add(link.paperlessDocumentId); @@ -382,7 +352,7 @@ export async function getSourceReport( } } - // Step i: Build budget lines per invoice + // Step h: Build budget lines per invoice const budgetLinesByInvoiceId = new Map(); for (const [iblId, lineContrib] of railALineContributions) { const invoiceId = lineContrib.invoiceId; @@ -406,7 +376,7 @@ export async function getSourceReport( budgetLinesByInvoiceId.set(invoiceId, lines); } - // Step j: Fetch deposits for each invoice (unfiltered by status, but filtered by source tag) + // Step i: Fetch deposits for each invoice (unfiltered by status, but filtered by source tag) const depositsByInvoiceId = new Map(); for (const invoiceId of allInvoiceIds) { const deposits = db @@ -470,7 +440,7 @@ export async function getSourceReport( }); } - // Step k: Query unallocated invoices (excluding those with tagged deposits) + // Step j: Query unallocated invoices (excluding those with tagged deposits) const unallocatedInvoices: SourceReportUnallocatedInvoice[] = []; const unallocRows = db.all<{ invoice_id: string; diff --git a/wiki b/wiki index f3101b524..a9b6e9ef3 160000 --- a/wiki +++ b/wiki @@ -1 +1 @@ -Subproject commit f3101b524dda76c264b744f2e8f083b9a62ceeb3 +Subproject commit a9b6e9ef3925b186309ac203d873d533fd61ec45 From 5acd51a1daa8ba9bbee1a5fca3ba5dfd5aca7b3f Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Sun, 2 Aug 2026 15:50:38 +0200 Subject: [PATCH 2/2] test(reports): add change-detecting proof-of-funds tier fixture (#1930) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing AC3 proof-of-funds assertions were not change-detecting — they would have passed identically on beta's pre-#1930 stage-derivation logic. Adds a discriminating fixture: a deposit-only invoice (no budget line) whose sole tagged deposit is 'claimed', which the old code's "no split entry" branch would have kept via railBContributions alone, independent of report type. Verified to fail against the inlined pre-#1930 logic and pass against current. Also aligns wiki/API-Contract.md's attachmentType field wording ("Document stage" -> "Attachment tier") with the tier rule documented below it. Co-Authored-By: Claude dev-team-lead Co-Authored-By: Claude qa-integration-tester Co-Authored-By: Claude backend-developer Co-Authored-By: Claude product-architect Co-Authored-By: Claude product-owner --- .../product-architect/story-reviews.md | 32 +++++++++++++++++++ .claude/agent-memory/product-owner/MEMORY.md | 2 +- .../product-owner/bank-report-wizard.md | 10 +++++- .../qa-integration-tester/MEMORY.md | 2 +- .../story-1930-attachment-tier.md | 22 +++++++++++++ .../src/services/sourceReportService.test.ts | 29 +++++++++++++++++ wiki | 2 +- 7 files changed, 95 insertions(+), 4 deletions(-) diff --git a/.claude/agent-memory/product-architect/story-reviews.md b/.claude/agent-memory/product-architect/story-reviews.md index d9276da6b..808e18eb6 100644 --- a/.claude/agent-memory/product-architect/story-reviews.md +++ b/.claude/agent-memory/product-architect/story-reviews.md @@ -342,3 +342,35 @@ Two review lessons worth keeping: Four rounds total. Trajectory was right each time; each round bounded something real and revealed the next layer. Worth remembering before pushing for a five-round rewrite: the arc converged. + +## Story #1930 — Attachment tier rules per report type (PR #1942, APPROVED 2026-08-02) + +Per-invoice stage matching (invoice status slice + deposit split + `targetStatuses`) replaced by a +pure two-arg predicate in `server/src/services/shared/attachmentTierUtils.ts`: +tier `quotation`1 < `deposit`2 < `invoice`3; floors budget-overview 1 / claim 2 / proof-of-funds 3. +`attachmentType: null` = tier 3 (product ruling — nulls are legacy/ambiguous, and a silently-dropped +attachment is unrecoverable while an over-included one is deselectable). + +Three durable conclusions: + +1. **Server-local vs `@cornerstone/shared` — the relocation trigger.** #1916's drift came from *two + implementations*, not from server-local placement. Relocating a rule with one implementation and + zero client callers reduces nothing and adds build-order coupling. Sharper: for the client to need + this predicate, the server would have to ship *unfiltered* documents — which AC7 forbids. So + client-side need is a contract violation, not a future extension. Rule to reuse: + **move to `@cornerstone/shared` iff a client module must evaluate the rule against data the server + has not already filtered.** +2. **`Record` object literal is the right exhaustiveness mechanism** (fails the build when + the union grows; `Partial<>` degrades to `undefined`, `switch` needs a `never` guard). Its residual + hole — an out-of-enum DB value indexing to `undefined`, and `undefined >= floor` silently excluding + from *every* report — is closed here by a real `CHECK` in migration `0042`, not by Drizzle's + compile-time `text(..., {enum})`. **Always check whether the migration has the CHECK before calling + a cast-fed `Record` lookup safe.** +3. **Reports are computed on read** — no report table in `schema.ts`, PDF built client-side per + invocation (ADR-034). Changing report filtering has no persisted blast radius, no cache, no backfill. + +Findings posted: MEDIUM (pre-existing, follow-up) `ReportWizardPage.handleUseCaseChange` never clears +`report`/`sourceId`, so changing the use case and clicking straight through step 2 reaches step 3 with +a report fetched under the *previous* use case — the tier rule is right, the wizard just holds output +from the wrong invocation. LOW: `wiki/API-Contract.md:3625` still says "Document stage" four lines +above the tier tables that retire that word. diff --git a/.claude/agent-memory/product-owner/MEMORY.md b/.claude/agent-memory/product-owner/MEMORY.md index 56a666d8f..80ee1c860 100644 --- a/.claude/agent-memory/product-owner/MEMORY.md +++ b/.claude/agent-memory/product-owner/MEMORY.md @@ -37,7 +37,7 @@ Full detail in [standalone-bugs-and-stories.md](standalone-bugs-and-stories.md) - Auto-itemize: #1545/#1546/#1547 mini-epic (2026-05-21), #1600 (2026-05-26), **#1833 duplicate budget lines on commit retry (2026-07-07)** - Diary: #1426 critical photo data loss (2026-05-15) - Photo: #1723 lightbox picker UX (2026-06-16) -- **Bank Report Wizard mini-epic** (no parent epic): #1876 refunds (PR #1880) → #1877 contact/household/attachment typing (PR #1883) → #1878 report backend → #1879 wizard+PDF (PR #1887, CHANGES_REQUIRED then **APPROVED** round 2, 2026-07-30). Domain decisions, contract facts (refund sign, `isSplit`, `totalAmount` scope) and deferrals in [bank-report-wizard.md](bank-report-wizard.md). **Refinement Round 2** (2026-07-31, no parent epic, all Todo): #1898 report table refinements (PR #1902, merged) → #1899 settings step + report language (PR #1903, PO review 2026-07-31: **APPROVED w/ 1 MUST FIX** — 5-step wizard, `getFixedT`/`createFormatters` threading and en/de report output all verified; AC 2.2 default-locale seeding is stale on hard load, see [pr-review-patterns.md](pr-review-patterns.md) `useState(contextValue)` entry) → #1900 editable HTML preview (PR #1909, round 1 COMMENT + 4 MUST FIX, **round 2 2026-07-31 APPROVED** — all 4 verified on disk: local `composes` classes, `--font-weight-medium`, `sharedStyles.srOnly`, translated `resetFieldAriaLabel` at all 9 sites w/ en+de parity; stylelint exit 0. Note: `gh pr review --approve` fails when PO authored the PR context — post verdict via `gh pr comment` with explicit Verdict line. Judgment rulings: signature-derived-from-sender ACCEPTED, mark-claimed-generates-no-PDF ACCEPTED as vacuous, per-field reset ACCEPTED, AC 4.6 rendered-preview assertion ACCEPTED as documented deviation — Playwright headless has no PDF viewer plugin, so the E2E asserts the CSP `frame-src` contract instead; **mixed-language mobile cards ACCEPTED** — see [bank-report-wizard.md](bank-report-wizard.md) "artifact content vs. edit affordance") → #1901 AI usage/cover-letter generation (PR #1916, PO review 2026-07-31: **CHANGES_REQUIRED** — 3 blocking numeric-accuracy defects in the LLM prompt inputs: `/100` on major-unit amounts, `Math.round` to whole euros, per-invoice amount ignoring `excludedLineIds`; + 2 MUST FIX: extraction-flavoured shared LLM error copy, uncommitted wiki API-Contract section. All 6 AC sections otherwise met; entity-level linked-item description deviation ACCEPTED. New defect class recorded in [pr-review-patterns.md](pr-review-patterns.md) "LLM/prompt-assembly defects". **Round 2 on `b70d821b`: APPROVED** — all 5 findings fixed and verified on disk; `prompts.test.ts` gained a dedicated ×100 regression-guard block (98/98 pass locally); per-invoice cents-rounding now makes server math identical to client `applyLineExclusions`; wiki pushed at `254db1d`; the 9 removed test lines were a stale #1915 header note, not a weakened assertion). **Follow-ups consolidated into #1917** (tech-debt, Should Have, Backlog): architect M1–M4 + L1/L2/L3/L5, the `Konstruktionsprojekt`→`Bauprojekt` prompt nit, and the approved `KI` glossary entry. M2 (extract `computeIncludedTotal` to `@cornerstone/shared`) is the headline — the client/server duplication already drifted once and caused the #1916 blocking bug. Open: **#1891** user-verification follow-up (Todo, PR #1894 **APPROVED** 32/32 round 2, 2026-07-30 — 2 wiki MUST FIX outstanding); **#1888** stage-matched attachment indicator (Backlog, blocked-by #1879); **#1895** HIGH claim close-out cross-source sweep, **#1896** quotation-deposit 409 (blocked-by #1895), **#1897** deposit-blind drill-down — all Backlog, from the #1891 architect audit; **#1910** `lang` attribute on report-language preview content (Backlog, a11y follow-up from #1909 round 2); E2E shard 5 pre-existing flake must be triaged before promoting to `main`. **Refinement Round 3** (2026-08-02, from user PDF inspection + wizard walkthrough, all Todo, for `/batch-develop`): **#1929** PDF layout robustness (bug, Must Have — column widths, `dontBreakRows`, header clipped by 40pt top margin; **PR #1935 CHANGES_REQUIRED ×2, AC2-vs-AC4 conflict ruled 2026-08-02: precedence ladder I1 no-loss > I2 no-clip > I3 row-whole > I4 no-word-break; AC2/3/4 rewritten, AC12–AC14 added; 600-char target**), **#1930** attachment tier rules per report type (quotation→deposit→invoice; null = tier `invoice`; supersedes #1888's design question), **#1931** single "Enhance with AI" button + purpose-focused prompt (takes the `Konstruktionsprojekt` nit off #1917), **#1932** cover letter overhaul (folds in #1925, reverses #1909's derived-signature acceptance), **#1933** Select Invoices step UI fixes. Rulings in [bank-report-wizard.md](bank-report-wizard.md) §"Refinement Round 3". **#1929 CLOSED 2026-08-02** — PR #1935 merged (squash `1c5aa62c`) after **4 rounds**; both reviewers measured by real render+rasterize. 5 follow-ups filed: **#1937** German header labels break mid-word (bug, Todo, translator fast-follow — widening measured and rejected), **#1938** running-header `generated at` label with no timestamp on pages 2+ (bug, Todo, **pre-existing**), **#1939** reportPdf geometry hygiene (tech-debt, Todo, **blocks #1932** — `HEADER_ROW_HEIGHT`→`_MAX` 68pt vs measured 45.81pt, char-advance comment scoping, `PDF_STYLES` relocation), **#1940** continuation rows read as broken (could have, Backlog), **#1941** override fields have no `maxLength` (could have, Backlog). `markerText`+`invoiceNumber` folded into #1939 as documentation-only; vendor-name mid-word break recorded as accepted limitation in #1937. Detail in [bank-report-wizard.md](bank-report-wizard.md) §"#1929 closed". +- **Bank Report Wizard mini-epic** (no parent epic): #1876 refunds (PR #1880) → #1877 contact/household/attachment typing (PR #1883) → #1878 report backend → #1879 wizard+PDF (PR #1887, CHANGES_REQUIRED then **APPROVED** round 2, 2026-07-30). Domain decisions, contract facts (refund sign, `isSplit`, `totalAmount` scope) and deferrals in [bank-report-wizard.md](bank-report-wizard.md). **Refinement Round 2** (2026-07-31, no parent epic, all Todo): #1898 report table refinements (PR #1902, merged) → #1899 settings step + report language (PR #1903, PO review 2026-07-31: **APPROVED w/ 1 MUST FIX** — 5-step wizard, `getFixedT`/`createFormatters` threading and en/de report output all verified; AC 2.2 default-locale seeding is stale on hard load, see [pr-review-patterns.md](pr-review-patterns.md) `useState(contextValue)` entry) → #1900 editable HTML preview (PR #1909, round 1 COMMENT + 4 MUST FIX, **round 2 2026-07-31 APPROVED** — all 4 verified on disk: local `composes` classes, `--font-weight-medium`, `sharedStyles.srOnly`, translated `resetFieldAriaLabel` at all 9 sites w/ en+de parity; stylelint exit 0. Note: `gh pr review --approve` fails when PO authored the PR context — post verdict via `gh pr comment` with explicit Verdict line. Judgment rulings: signature-derived-from-sender ACCEPTED, mark-claimed-generates-no-PDF ACCEPTED as vacuous, per-field reset ACCEPTED, AC 4.6 rendered-preview assertion ACCEPTED as documented deviation — Playwright headless has no PDF viewer plugin, so the E2E asserts the CSP `frame-src` contract instead; **mixed-language mobile cards ACCEPTED** — see [bank-report-wizard.md](bank-report-wizard.md) "artifact content vs. edit affordance") → #1901 AI usage/cover-letter generation (PR #1916, PO review 2026-07-31: **CHANGES_REQUIRED** — 3 blocking numeric-accuracy defects in the LLM prompt inputs: `/100` on major-unit amounts, `Math.round` to whole euros, per-invoice amount ignoring `excludedLineIds`; + 2 MUST FIX: extraction-flavoured shared LLM error copy, uncommitted wiki API-Contract section. All 6 AC sections otherwise met; entity-level linked-item description deviation ACCEPTED. New defect class recorded in [pr-review-patterns.md](pr-review-patterns.md) "LLM/prompt-assembly defects". **Round 2 on `b70d821b`: APPROVED** — all 5 findings fixed and verified on disk; `prompts.test.ts` gained a dedicated ×100 regression-guard block (98/98 pass locally); per-invoice cents-rounding now makes server math identical to client `applyLineExclusions`; wiki pushed at `254db1d`; the 9 removed test lines were a stale #1915 header note, not a weakened assertion). **Follow-ups consolidated into #1917** (tech-debt, Should Have, Backlog): architect M1–M4 + L1/L2/L3/L5, the `Konstruktionsprojekt`→`Bauprojekt` prompt nit, and the approved `KI` glossary entry. M2 (extract `computeIncludedTotal` to `@cornerstone/shared`) is the headline — the client/server duplication already drifted once and caused the #1916 blocking bug. Open: **#1891** user-verification follow-up (Todo, PR #1894 **APPROVED** 32/32 round 2, 2026-07-30 — 2 wiki MUST FIX outstanding); **#1888** stage-matched attachment indicator (Backlog, blocked-by #1879); **#1895** HIGH claim close-out cross-source sweep, **#1896** quotation-deposit 409 (blocked-by #1895), **#1897** deposit-blind drill-down — all Backlog, from the #1891 architect audit; **#1910** `lang` attribute on report-language preview content (Backlog, a11y follow-up from #1909 round 2); E2E shard 5 pre-existing flake must be triaged before promoting to `main`. **Refinement Round 3** (2026-08-02, from user PDF inspection + wizard walkthrough, all Todo, for `/batch-develop`): **#1929** PDF layout robustness (bug, Must Have — column widths, `dontBreakRows`, header clipped by 40pt top margin; **PR #1935 CHANGES_REQUIRED ×2, AC2-vs-AC4 conflict ruled 2026-08-02: precedence ladder I1 no-loss > I2 no-clip > I3 row-whole > I4 no-word-break; AC2/3/4 rewritten, AC12–AC14 added; 600-char target**), **#1930** attachment tier rules per report type (quotation→deposit→invoice; null = tier `invoice`; supersedes #1888's design question) — **PR #1942 APPROVED round 1, 2026-08-02**, all 11 AC met, 80/80 green; **but #1943** (bug, **Must Have**, Todo, 2026-08-02) — `handleUseCaseChange` never clears `report`/`sourceId`, so budget-overview→claim carries a stale report and can embed **quotation-tier docs in a claim PDF**, reaching #1930 AC2's forbidden outcome by a route AC2 doesn't cover; ruled: clear `sourceId` too — **PR #1942 APPROVED round 1, 2026-08-02**, all 11 AC met, 80/80 green; **#1888 body re-scoped to indicator presentation only at review time** (it was still stale), **#1931** single "Enhance with AI" button + purpose-focused prompt (takes the `Konstruktionsprojekt` nit off #1917), **#1932** cover letter overhaul (folds in #1925, reverses #1909's derived-signature acceptance), **#1933** Select Invoices step UI fixes. Rulings in [bank-report-wizard.md](bank-report-wizard.md) §"Refinement Round 3". **#1929 CLOSED 2026-08-02** — PR #1935 merged (squash `1c5aa62c`) after **4 rounds**; both reviewers measured by real render+rasterize. 5 follow-ups filed: **#1937** German header labels break mid-word (bug, Todo, translator fast-follow — widening measured and rejected), **#1938** running-header `generated at` label with no timestamp on pages 2+ (bug, Todo, **pre-existing**), **#1939** reportPdf geometry hygiene (tech-debt, Todo, **blocks #1932** — `HEADER_ROW_HEIGHT`→`_MAX` 68pt vs measured 45.81pt, char-advance comment scoping, `PDF_STYLES` relocation), **#1940** continuation rows read as broken (could have, Backlog), **#1941** override fields have no `maxLength` (could have, Backlog). `markerText`+`invoiceNumber` folded into #1939 as documentation-only; vendor-name mid-word break recorded as accepted limitation in #1937. Detail in [bank-report-wizard.md](bank-report-wizard.md) §"#1929 closed". ## Requirements Coverage diff --git a/.claude/agent-memory/product-owner/bank-report-wizard.md b/.claude/agent-memory/product-owner/bank-report-wizard.md index 09b976763..da48bc259 100644 --- a/.claude/agent-memory/product-owner/bank-report-wizard.md +++ b/.claude/agent-memory/product-owner/bank-report-wizard.md @@ -157,7 +157,7 @@ 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`. +- **#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. @@ -166,6 +166,14 @@ Source: user inspection of downloaded report PDFs + a wizard walkthrough after # - **`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.** +- **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. diff --git a/.claude/agent-memory/qa-integration-tester/MEMORY.md b/.claude/agent-memory/qa-integration-tester/MEMORY.md index 46182e4f0..34c250f07 100644 --- a/.claude/agent-memory/qa-integration-tester/MEMORY.md +++ b/.claude/agent-memory/qa-integration-tester/MEMORY.md @@ -15,7 +15,7 @@ ## Recent bug/story notes (2026-08) -- [Story #1930 — attachment tier rules](story-1930-attachment-tier.md) (2026-08-02) — new `attachmentTierUtils.test.ts` (100% cov, table+explicit-null pattern); sourceReportService.test.ts 16a/b/c/e → AC1/AC2/AC3/AC5; cross-report-type test needs a fresh invoice per report type (no single status is in all 3 target slices). +- [Story #1930 — attachment tier rules](story-1930-attachment-tier.md) (2026-08-02) — new `attachmentTierUtils.test.ts` (100% cov, table+explicit-null pattern); sourceReportService.test.ts 16a/b/c/e → AC1/AC2/AC3/AC5; cross-report-type test needs a fresh invoice per report type; round 2 added a genuinely discriminating proof-of-funds fixture (deposit-only invoice, tagged+claimed deposit) after PO flagged the original AC3/16 pof blocks weren't change-detecting — inline-swap isolation technique to prove it fails on old logic. - [Issue #1929 round 2 — real-render pdfmake technique](story-1929-round2-real-render-technique.md) (2026-08-02, PR #1935) — `_calcWidth`/`.positions[].pageNumber` are readable after a real `getBlob()` on a held content reference (round-1's "inaccessible" comment was wrong, verified against pdfmake@0.3.11 source); measured Usage column widths (138.28pt/186.78pt, both locales, both shapes); git-stash/backup technique to prove tests genuinely fail on unpatched code; tighter layouts break old page-count fixture assumptions (scale fixtures up, don't weaken assertions); `expect(async () => {}).not.toThrow()` is a no-op anti-pattern. - [Story #1923 — report table cleanup](story-1923-report-table-cleanup.md) (2026-08-02) — unnumbered shared †/‡ markers, isDeposit/isClaim/areaText fixture ripple across 8 report test files, overviewPdf allocated-cell-is-always-an-array-of-runs gotcha, worktree `node_modules/@cornerstone/shared` symlink pointing at a differently-branched base repo (false-positive `tsc` errors; trust jest). - [Bugs #1895/#1896/#1918 — claim/deposit scope fixes](bugs-1895-1896-1918-claim-deposit-scope.md) (2026-08-01) — `markInvoicesClaimed` gained `sourceId`+required `depositIds` params (cross-source claim guard, decoupled sweep, quotation+sweepable-deposit no longer 409s); `getSourceReport` drops zero-portion `budgetLines[]` on `claim` reports only; text-content query collision gotcha (banner text contains data also shown elsewhere on page). diff --git a/.claude/agent-memory/qa-integration-tester/story-1930-attachment-tier.md b/.claude/agent-memory/qa-integration-tester/story-1930-attachment-tier.md index d2c9901a5..43235ddb1 100644 --- a/.claude/agent-memory/qa-integration-tester/story-1930-attachment-tier.md +++ b/.claude/agent-memory/qa-integration-tester/story-1930-attachment-tier.md @@ -32,3 +32,25 @@ scoped so `result.invoices` has exactly 1 entry (no need to filter by invoiceId) AC6 regression guard (scenarios 8-15b: status-slice selection, isSplit, refund-adjustment, zero-drop, unallocated) untouched and confirmed still green — no evidence the change leaked beyond `documents[]` filtering. + +**Round 2 (PO review follow-up, same day)**: PO flagged that the proof-of-funds blocks of +scenario 16 and the AC3 test were NOT change-detecting — a `claimed` no-deposit invoice hits the +old stage-derivation's default empty-stages branch too, so both would have passed on `beta` +unchanged. Added a genuinely discriminating fixture: `AC3 (discriminating)` — a **deposit-only** +invoice (no `invoice_budget_line` at all) whose sole **tagged** deposit is `claimed`, with a +`deposit`-typed document link. Under the OLD stage-derivation (`splitByDepositsExcludingTagged` +returns no entry when there's no Rail A row for the invoice → falls into the "no split" branch → +`stages.add('deposit')` fires purely because `railBContributions.has(invoiceId)`, independent of +report type) → old code would have KEPT the doc. Under the tier rule, proof-of-funds floor = +`invoice`(3) > `deposit`(2) → excluded. 81 tests total (was 80). + +**Isolation technique used to prove it's discriminating** (since `attachmentTierUtils.ts` doesn't +exist on `beta`, a straight file swap fails at import resolution, not at the assertion): back up +the current (fixed) `sourceReportService.ts` to `/tmp`, edit the working copy in place to inline +the pre-#1930 stage-derivation logic (re-import `splitByDepositsExcludingTagged` from +`depositAggregateUtils.js`, replace the `isDocumentIncludedForReportType` call site with the old +`stages` Set computation copied from `git show origin/beta:...`), run *only* the new test via +`npx jest ... -t "AC3 \(discriminating\)"`, confirm it fails with the exact wrong-inclusion +diff, then `cp` the backup back over the working file and re-verify `git diff` is empty before +re-running the full suite. This isolates one specific fixture's discriminating power without +needing a parallel beta checkout or touching any other test. diff --git a/server/src/services/sourceReportService.test.ts b/server/src/services/sourceReportService.test.ts index 79f03e36a..3f4f31884 100644 --- a/server/src/services/sourceReportService.test.ts +++ b/server/src/services/sourceReportService.test.ts @@ -574,6 +574,35 @@ describe('sourceReportService', () => { expect(result.invoices[0]!.documents).toEqual([]); }); + // The AC3 test above is not change-detecting: a 'claimed' no-deposit invoice hit the old + // stage-derivation's default (no split entry -> no railBContributions entry -> empty + // stages set) and would have excluded the 'deposit' doc on `beta` too. This fixture is + // the discriminating one: a deposit-only invoice (no invoice_budget_line at all) whose + // sole, tagged deposit is 'claimed' — in the OLD stage-derivation this hits the "no split + // entry" branch that adds the 'deposit' stage purely because `railBContributions` has an + // entry for the invoice, independent of the report type. That would have kept the + // 'deposit'-typed document in a proof-of-funds report on `beta`. Under the tier rule, + // proof-of-funds' floor is `invoice` (3) and `deposit` is tier 2, so the document must be + // excluded regardless of the deposit's own status. Verified change-detecting against the + // pre-#1930 filtering logic — see qa-integration-tester agent memory for the isolation + // technique used. + it('AC3 (discriminating): a claimed tagged deposit does not resurrect a deposit-typed document in a proof-of-funds report', async () => { + const sourceId = insertSource(); + const vendorId = insertVendor(); + const invId = insertInvoice(vendorId, { status: 'claimed', amount: 500 }); + insertDeposit(invId, { + amount: 500, + status: 'claimed', + entryType: 'deposit', + budgetSourceId: sourceId, + }); + insertDocumentLink(invId, 1, 'deposit'); + + const result = await getSourceReport(db, 'proof-of-funds', sourceId, PAPERLESS_DISABLED); + expect(result.invoices).toHaveLength(1); + expect(result.invoices[0]!.documents).toEqual([]); + }); + it('AC5: invoice status is irrelevant to document selection — quotation-status and paid-status invoices filter identically', async () => { const sourceId = insertSource(); const vendorId = insertVendor(); diff --git a/wiki b/wiki index a9b6e9ef3..cd4023e7c 160000 --- a/wiki +++ b/wiki @@ -1 +1 @@ -Subproject commit a9b6e9ef3925b186309ac203d873d533fd61ec45 +Subproject commit cd4023e7ccd0549aae810d551c3d6cb254dade3a