feat(reports): filter report attachments by document tier per report type (#1930)

[steilerDev]

Summary

• Documents now form an ordered evidentiary tier — quotation (1) -> deposit (2) -> invoice (3) — and each report type declares a tier floor: budget-overview = 1 (embeds all), claim = 2 (never embeds a quotation), proof-of-funds = 3 (invoices only).
• Replaces 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. The rule now depends solely on report type + the document's own type, expressed once in server/src/services/shared/attachmentTierUtils.ts.
• Untagged (null) attachmentType documents are treated as the strongest tier (invoice), so existing pre-Source contact fields, household sender setting & document attachment typing #1877/untyped links are never silently dropped from claim or proof-of-funds reports.
• wiki/API-Contract.md updated with the new tier model, replacing the stale document-stage-rule description.

Fixes #1930

Test plan

• Unit tests pass (attachmentTierUtils.ts 100% stmts/branches/funcs/lines; sourceReportService.ts 100% stmts/funcs/lines, 93.18% branch — pre-existing gaps unrelated to this change)
• Integration tests pass (80/80 in the two touched suites, verified locally)
• CI Quality Gates pass (typecheck, tests, build, audit)
• Note: E2E shards 5 (invoices.spec.ts:841) and 6 (dashboard.spec.ts:516) are expected to fail — pre-existing, unrelated flakes (also failing on PR feat(reports): report table cleanup — shared footnotes, deposit labels, claim metadata, total-only summary, usage area (#1923) #1924). E2E Gates is not required on beta.

Co-Authored-By: Claude dev-team-lead noreply@anthropic.com
Co-Authored-By: Claude backend-developer noreply@anthropic.com
Co-Authored-By: Claude qa-integration-tester noreply@anthropic.com
Co-Authored-By: Claude product-owner noreply@anthropic.com
Co-Authored-By: Claude product-architect noreply@anthropic.com

[steilerDev]

[product-owner] Requirements review of PR #1942 against story #1930 (head 4dfce4b8).

Verdict: APPROVED

I wrote #1930, so I checked every AC against the code and tests rather than against the summary. All 11 pass. Verified locally: attachmentTierUtils.test.ts + sourceReportService.test.ts → 80/80 green.

Acceptance criteria

AC	Verdict	Evidence
1 — Tier floors	MET	REPORT_TYPE_TIER_FLOOR maps budget-overview→1, claim→2, proof-of-funds→3, exactly the issue's table. Table-driven unit test covers all 9 (report type × typed attachment) combinations; scenario 16 (AC1) proves it end-to-end through getSourceReport for all three report types with a 4-document invoice.
2 — Quotations never reach a claim report	MET	claim floor is ATTACHMENT_TIER.deposit (2); quotation is tier 1, so isDocumentIncludedForReportType('claim','quotation') is false. Named test AC2 asserts documents is [] for a paid invoice (squarely inside the claim slice) whose only document is a quotation. Because the filter is server-side, the exclusion propagates to the step-3 indicator, the appendix and the footnotes automatically — see AC7/AC8 below.
3 — Deposits never reach proof-of-funds	MET	proof-of-funds floor is tier 3; deposit is tier 2. Named test AC3 uses a claimed invoice (the proof-of-funds slice) with a single deposit-typed document and asserts the invoice is still present with documents: [] — i.e. the "no document" treatment, not a dropped invoice.
4 — Null is tier invoice	MET	attachmentType === null ? ATTACHMENT_TIER.invoice : …. Implemented as I ruled. Three individually named tests (null … budget-overview / claim / proof-of-funds), deliberately kept out of the table with a comment saying why. That is exactly the treatment I asked for — this is the rule most likely to be silently inverted by a well-meaning refactor, and a reviewer deleting one of these three lines now has to do it on purpose. The null case also rides along in scenario 16 (AC1) for all three types.
5 — Invoice status irrelevant	MET	Structurally, not just by test: isDocumentIncludedForReportType(type, link.attachmentType) takes only the report type and the link's own type. splitsByInvoiceId is gone from the document path; targetStatuses and railBContributions survive only in the amount/selection code (lines 64–77, 144, 226–257, 465). Test AC5 puts a quotation-status and a paid-status invoice side by side with identically typed documents and asserts identical survivors.
6 — Only documents changed	MET	Diff on sourceReportService.ts is confined to the import block and the step-g/h document-filter block, plus step-letter renumbering in comments. Regression guards scenario 8–15b (status-slice selection, isSplit, zero-drop, refund adjustment, unallocated), 16d, 17–21 are untouched and passing.
7 — One source of truth	MET, and I re-verified the client explicitly. client/src/lib/reportPdf/merge.ts iterates invoice.documents unconditionally (L85, L90, L200); ReportInvoiceList.tsx L149 lights the indicator on documents.length > 0. No second filter was introduced anywhere on the client. It is structurally impossible for step 3 and the PDF to disagree.
8 — Appendix and footnotes follow the filter	MET	Appendix numbering (merge.ts L83–113) increments only over invoice.documents, so filtered-out documents never consume a number. Every skippedDocuments.push is on a fetch failure or an invalid PDF (footnoteFetchFailed / footnoteInvalidPdf) — tier-excluded documents are never candidates and produce no skipped footnote, as specified.
9 — Tier model expressed once	MET	server/src/services/shared/attachmentTierUtils.ts is the only definition of both the ordering and the floors, with a doc comment forbidding a second site. Grep confirms no other hard-coded floor.
10 — Documented	MET	wiki/API-Contract.md @ a9b6e9e (pushed to master) replaces the old four-bullet stage rule with both tier tables, the null = tier invoice ruling with its rationale, the explicit "does not consult invoice status / deposit split / target-status slice" statement, and a pointer to the single implementation site. It says what I ruled, not a paraphrase of it.
11 — Tests	MET	Per report type: all three typed documents, a null-typed document, a mixed-type invoice (scenario 16), and an invoice whose every document is filtered out (AC2 for claim, AC3 for proof-of-funds). Assertions are on the tier outcome (documents[].attachmentType), never on the removed stage derivation.

On the QA deviation in the AC1 scenario (fresh invoice per report-type block)

I agree the AC1 contract is fully covered and not diluted. The reasoning is sound and I'd have ruled the same way: no single invoice status sits in all three target slices at once (proof-of-funds requires claimed, which the claim slice excludes), so "one shared invoice queried three times" is not constructible without changing the invoice-selection variable the test isn't about. Splitting into three fixtures isolates exactly one variable per block — report type — and status-invariance of the outcome is proven separately and directly by AC5. The three blocks use an identical 4-document fixture built by one helper, so the comparison across report types is still apples-to-apples.

Non-blocking observations

I1 (informational, no action required). Two of the proof-of-funds assertions are not change-detecting against the removed implementation: for a claimed-status invoice with no deposits, the old stage derivation also produced stages = {invoice}, so scenario 16's proof-of-funds block and AC3 would both have passed on beta too. The change detection for that report type lives in the unit test's floor assertion and in AC1's budget-overview and claim blocks (both of which fail on the old code). A fixture that would discriminate is a proof-of-funds report over an invoice with a claimed-status tagged deposit plus a deposit-typed document — old code adds the deposit stage, new code excludes it. Worth adding opportunistically if this area is touched again; not worth a round trip now, since the behaviour it would guard is already locked by the unit test.

I2 (done, not a request). #1930 ruled that #1888 stays open, re-scoped to indicator presentation only. Nothing in this PR conflicts with that — it touches no client code, and it removes rather than creates the "attached but not stage-matched" state #1888 originally proposed (non-qualifying documents no longer reach the client at all). #1888's text had not been updated, so I rewrote it just now: the attachmentType: null AC is replaced by a pointer to the ruling shipped here, the third indicator state is struck, a "no client-side filtering" AC is added to protect AC7, and coordination with #1933 (which restyles the same glyph) is called out.

Story #1930 is complete as specified. No scope creep: no attachment-selection UI, no change to the attachDocuments setting, no attachmentType backfill.

[steilerDev]

[product-architect] Architecture review — APPROVED

Verified against the Wiki API Contract, the AC set on #1930, and the surrounding report pipeline. No blocking findings. The change is genuinely smaller than the code it deletes: a per-invoice derivation with three inputs (invoice status slice, deposit split, targetStatuses) collapses into a pure two-argument predicate. That is the right direction, and it is the reason AC5 and AC9 fall out for free rather than needing to be enforced.

---

Answers to the six questions raised

1. Placement in server/src/services/shared/ rather than @cornerstone/shared — correct as-is

I endorse the dev-team-lead's call, but for a sharper reason than "nothing consumes it yet".

The #1916 drift was caused by two implementations of one rule, one per side. Relocating a rule that has exactly one implementation and zero client callers does not reduce drift risk — there is nothing to drift from. What it does add is a build-order coupling (shared → client → server) for a rule the client never evaluates.

More decisively: for the client to need the tier rule, the server would have to ship unfiltered documents plus enough context for the client to re-derive inclusion. That is precisely what AC7 forbids ("the filter is applied once, server-side, and both consumers read the filtered result"). So client-side need for this predicate is not a future extension — it is a contract violation. #1888 as re-scoped (indicator presentation over an already-filtered documents array) does not need it either.

Worth writing the relocation trigger down so a future reader doesn't re-litigate it: move to @cornerstone/shared iff a client module must evaluate the rule against data the server has not already filtered. Until that is true, the move is speculative and mildly contract-eroding.

2. Record<AttachmentType, number> / Record<SourceReportType, number> — right mechanism

Better than the alternatives for this shape. Partial<...> degrades a missing member into undefined at runtime instead of a compile error; a switch needs an explicit never exhaustiveness guard to get the same protection, which is more machinery for the same result. The Record literal fails the build the moment either union grows — which is exactly the failure you want, since a new attachment type with no tier is a semantic question a human has to answer.

One residual hole, and I checked whether it is live: ATTACHMENT_TIER[attachmentType] is reached through a as AttachmentType | null cast at sourceReportService.ts:315, so a DB value outside the enum would yield tier === undefined, and undefined >= floor is false — the document would be silently dropped from every report type, the exact failure mode the null ruling was designed to avoid. That path is currently unreachable: migration 0042_document_links_attachment_type.sql carries a real CHECK(attachment_type IN ('quotation','deposit','invoice') OR attachment_type IS NULL), not just a Drizzle compile-time enum. Informational, no change requested — but noting it here so that any future migration that relaxes that CHECK knows it re-opens a silent-drop path.

3. Dead-code precision — verified clean, both directions

Nothing still-needed went with the deletion:

• splitByDepositsExcludingTagged remains exported and used at budgetSourceService.ts:321 and internally at depositAggregateUtils.ts:607 and :669, with its own tests at depositAggregateUtils.test.ts:1348+. Only the now-unused import in sourceReportService.ts was dropped.
• railBContributions — still live (:226, :229, :257), correctly untouched.
• targetStatuses — still live (:144, :226, :465); it now drives invoice slicing only, never document filtering, which is the point of the change.
• isSplitMap — still live (:292–:294, :436), untouched.
• AttachmentType type import still required (:315, :422).

Nothing now-dead was left behind: npx eslint on the three changed server files is clean (no no-unused-vars), and the step-letter renumbering is contiguous a–j with no orphan or duplicate.

4. Behavioural blast radius — nothing persisted, filtering is purely per-request

• No report entity exists in server/src/db/schema.ts — reports are computed on read, never stored. No migration, no backfill, no cache invalidation surface.
• The PDF is generated client-side per invocation (ADR-034); reportPdf/merge.ts iterates invoice.documents (:85, :90, :200) from the live response, so appendix numbering and skipped-document footnotes follow the filter automatically — AC8 holds structurally, not by a second rule.
• allPaperlessDocIds now accumulates only surviving links, so the Paperless metadata batch narrows in step with the filter. No consumer reads metadata for an excluded document.
• No client-side tier filter was introduced: ReportInvoiceList.tsx still gates on documents.length > 0, and buildReportContent.getAttachmentNote derives its label from whatever survived. AC7 and AC9 confirmed by inspection, not just by grep count.
• E2E: e2e/tests/budget/reportWizardEditableContent.spec.ts:248 seeds attachmentType: 'invoice', which clears every tier floor. No existing E2E expectation encodes the old stage-matching behaviour.

5. Null-as-tier-3 — implementation is coherent, and the asymmetry is the right one

Every other site that touches attachmentType: null treats it as unknown, not as invoice, and that is correct:

• buildReportContent.getAttachmentNote (:92–:98) routes all-null documents to attachmentsNoteNoType_* — a bare count with no type claim.
• LinkedDocumentCard.tsx:90 renders no badge for null.
• documentLinkService.ts:131 normalizes to null for non-invoice entities (never reached here — the report query filters entityType = 'invoice' at :303).
• invoiceAutoItemizeService.ts:764 hard-sets 'invoice', consistent with the ruling's stated rationale.

So null is tier-invoice for inclusion and unknown for labelling. That is not an inconsistency — it is the correct split, and it means the ruling never causes the report to assert something about a document it doesn't know. The visible consequence is that a proof-of-funds report can carry a row noting "1 attachment" with no type named. That is the accepted trade-off, not a defect.

6. Wiki accuracy — matches the implementation

wiki/API-Contract.md at a9b6e9e (submodule ref correctly bumped on the branch) states both tables, the null ruling with its rationale, the "does not consult invoice status / deposit split / target-status slice" negative constraint, and names attachmentTierUtils.ts as the single site. I checked each row against ATTACHMENT_TIER / REPORT_TYPE_TIER_FLOOR — all six mappings agree. Line 3659 ("If an invoice has no documents or none survive filtering, the documents array is empty") correctly reconciles with the Paperless-degradation paragraph below it. No Deviation Log entry is warranted: this is a synchronized update, not a divergence.

---

Findings by severity

Critical / High: none.

Medium (non-blocking, pre-existing, out of scope for this PR — recommend a follow-up issue)

ReportWizardPage.tsx — stale report survives a use-case change. handleUseCaseChange (:196–:222) sets useCase and setMaxReachedStep(2), but never clears report or sourceId. Only handleSourceChange (:239) refetches. Since step 2's Next is gated on disabled={!sourceId} and sourceId is retained, a user who changes the use case and clicks straight through step 2 without re-selecting the source reaches step 3 holding a report fetched under the previous use case.

This is pre-existing — it already staled the invoice slice, allocated amounts, and totals, all of which are more visible than documents. But #1930 raises the consequence rather than creating it: the stale document set is what gets embedded into a PDF sent to a bank. Concretely, budget-overview → claim without re-clicking the source would embed quotations, which is precisely what AC2 exists to prevent. The tier rule itself is correct; the wizard can just be holding output from the wrong invocation of it.

Suggested fix for the follow-up: clear report (and reset reportStatus to 'loading') inside handleUseCaseChange's guardedUpdate callback, which also forces the step-2 re-selection that repopulates it. Related minor artifact of the same gap: step 2's Next calls setCurrentStep(3) without setMaxReachedStep(3), so after a use-case change the stepper renders step 3 while still reporting max-reached 2.

Low

wiki/API-Contract.md:3625 — the invoices[].documents[].attachmentType row still reads "Document stage: quotation, deposit, invoice, or null for untagged". "Stage" is the vocabulary this story retires, and it sits four lines above the tier tables that replace it. Suggest "Document tier" (or just "Attachment type") for consistency with the section below. Cosmetic; fold into any next wiki touch.

Informational

1. Unknown-attachment_type fallback yields silent exclusion from all report types. Unreachable today thanks to the CHECK constraint in migration 0042 — recorded so a future migration relaxing that constraint knows what it re-opens. See §2 above.
2. Test shape is right: attachmentTierUtils.test.ts locks the constants independently of the function (so a refactor cannot move both in lockstep and stay green) and names the three null cases individually rather than folding them into the parameterised table. sourceReportService.test.ts correctly asserts tier outcomes rather than the removed derivation internals, and the AC5 case genuinely varies invoice status while holding document type constant — that is the assertion that would catch a regression toward status-driven filtering. Coverage: attachmentTierUtils.ts 100% across all metrics; sourceReportService.ts 93.18% branch with the uncovered lines pre-existing Rail-B null guards.

---

Verdict: APPROVED. Contract, schema, wiki, and implementation agree; no persisted state is affected; the one medium finding is pre-existing wizard state management and belongs in its own issue rather than this PR.

[github-actions]

🎉 This PR is included in version 2.13.0-beta.43 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀