fix(reports): source-scoped claim close-out, quotation deposit-only claims, hide non-applicable claim lines (#1895, #1896, #1918)

[steilerDev]

Summary

• Bug: mark-as-claimed is invoice-scoped, not source-scoped — claims other budget sources' money #1895 — POST /api/source-reports/mark-claimed is now source-scoped: it requires sourceId and an explicit depositIds list. The deposit sweep is restricted to deposits that are untagged or tagged to sourceId, and an invoice only flips pending/paid → claimed when no other budget source still funds a budget line on it — otherwise a deposit-only close-out runs and the invoice keeps its current status. Excluding a line in the wizard now keeps that invoice's status untouched (its excluded portion stays claimable later) while still sweeping its own deposits.
• Bug: quotation-status invoice with a pending deposit enters the claim report and 409s the entire mark-claimed batch #1896 — A quotation-status invoice with a sweepable deposit tagged to the reporting source is now claimable via the same deposit-only close-out (its status stays quotation), so it no longer 409s and rolls back an otherwise-valid mark-claimed batch. When INVOICES_NOT_CLAIMABLE is still returned, the wizard now names the offending invoice number(s) instead of showing a generic error.
• Bug: claim report shows non-applicable budget lines for quotation invoices entering via an unclaimed deposit #1918 — Claim reports (getSourceReport) now drop budget lines whose contribution to the claim status slice rounds to zero — e.g. a quotation invoice funded only by an unclaimed deposit tagged to the source. This hides non-applicable line items from the wizard's expandable row and from the AI report-content generation input, while leaving allocatedAmount, totalAmount, and the deposits sub-table unaffected. budget-overview and proof-of-funds reports are unchanged.

Fixes #1895
Fixes #1896
Fixes #1918

Test plan

• Unit tests pass (server: sourceReportService.test.ts, sourceReports.test.ts, reportContentGenerationService.test.ts; client: sourceReportsApi.test.ts, ReportWizardPage.test.tsx) — 92 server + 108 client tests green, coverage 91.6–100% across touched files
• E2E: reportWizardExpansion.spec.ts extended with new scenarios for the cross-source sweep (Bug: mark-as-claimed is invoice-scoped, not source-scoped — claims other budget sources' money #1895), the quotation + pending-deposit mixed batch (Bug: quotation-status invoice with a pending deposit enters the claim report and 409s the entire mark-claimed batch #1896), and the hidden-items quotation invoice (Bug: claim report shows non-applicable budget lines for quotation invoices entering via an unclaimed deposit #1918); existing warning-copy regex updated to match the corrected claim-modal copy
• CI Quality Gates pass (typecheck, tests, build, audit)

Co-Authored-By: Claude dev-team-lead noreply@anthropic.com
Co-Authored-By: Claude backend-developer noreply@anthropic.com
Co-Authored-By: Claude frontend-developer noreply@anthropic.com
Co-Authored-By: Claude translator noreply@anthropic.com
Co-Authored-By: Claude qa-integration-tester noreply@anthropic.com
Co-Authored-By: Claude e2e-test-engineer noreply@anthropic.com

[steilerDev]

[ux-designer] UX review — scope: copy quality (2 new/reworded i18n strings) and error-banner pattern consistency. No CSS/component files touched by this PR, confirmed via diff.

Verdict: APPROVED

• sourceReports.claimFailedWithInvoices (EN/DE): clear, matches the existing claimFailed/claimSuccess tone and the established invoice(s) count-agnostic pattern already used elsewhere in this namespace. Renders through the pre-existing Step5Actions.tsx sharedStyles.formErrorBanner div — same banner claimError already used before this PR, so no parallel implementation introduced.
• confirmClaimExcludedItemsWarning reword (EN/DE): accurately reflects the corrected behavior (invoice keeps its current status rather than falsely claiming excluded portions in full) — good catch that the old copy was actively misleading about what the fix now does. DE correctly uses an en-dash (–) rather than the EN em-dash (—), consistent with German typographic convention; terminology (einreichen/Einreichungsstatus) stays aligned with the existing claimedDate/claimSuccess glossary usage.
• Minor/informational, not blocking: the DE claimFailedWithInvoices string opens with "Konnte nicht als eingereicht markiert werden: …" without an explicit Es subject before the colon — slightly terser than the existing claimFailed string's "Rechnungen konnten nicht …" phrasing. Still understandable and mirrors a common list-style UI copy pattern; leaving as an optional refinement rather than a blocker.

No token/visual regressions possible here since no CSS or layout files are in this diff.

Co-Authored-By: Claude ux-designer (Sonnet 4.6) noreply@anthropic.com

[steilerDev]

[security-engineer]

Verdict: APPROVE (no Critical/High findings)

Scope: server/src/routes/sourceReports.ts and server/src/services/sourceReportService.ts (source-scoped claim close-out, #1895/#1896/#1918). Reviewed against the requested focus areas.

1. New batched SQL in markInvoicesClaimed — parameterization: OK

The otherSourceInvoiceIds query (sourceReportService.ts step d) uses the same safe pattern already established elsewhere in this file (splitRows, unallocRows): sql.join(invoiceIds.map((id) => sql\${id}`), sql`, `)for theIN (...)list and${sourceId}for the scalar comparison. Drizzle'ssql` tagged template parameterizes every interpolation — no string concatenation, no raw user input reaches the query text. No injection surface.

2. sourceId / depositIds request fields

• Schema: sourceId (string, minLength 1), invoiceIds (array, minItems 1), depositIds (array) are all required with additionalProperties: false — consistent with the route's existing convention. Minor informational note: unlike the sibling /generate-content route (added in #1901) which caps maxLength/maxItems on its array/string fields, mark-claimed's sourceId/invoiceIds/depositIds have no upper bounds. Not a regression introduced by this PR (invoiceIds already lacked a cap pre-PR) and bounded in practice by Fastify's default body size limit, but worth aligning for consistency in a future pass.
• Authorization model: Route only checks request.user is present (both Admin/Member allowed), matching the existing convention for this endpoint and the single-household trust model (no per-source ownership/ACL exists anywhere in the app — any authenticated user can already read/write all budget data). No cross-tenant concern.
• depositIds sweep scope — Low finding: Server-side, sweepEligibleDeposits is derived purely by looking up the client-supplied depositIds and filtering to budgetSourceId === null || budgetSourceId === sourceId (sourceReportService.ts:554-563). There is no check that a swept deposit's parent invoice is actually related to the invoiceIds being claimed (or to sourceId via a budget line) — Step f sweeps every sweep-eligible deposit regardless of whether its invoice appears anywhere in invoiceIds/allInvoiceIds's claim intent. A caller bypassing the UI could submit depositIds for deposits belonging to a completely unrelated invoice (as long as untagged or already tagged to the given sourceId) and have them flipped to claimed.
  • In practice this isn't reachable through the shipped UI: ReportWizardPage.tsx's handleMarkClaimed derives depositIds exclusively from report.invoices (already scoped to sourceId by getSourceReport), so the frontend never sends deposits outside the report's scope.
  • Given the app's trust model (Admin and Member both already have unrestricted read/write over all budget data — there's no per-user or per-source ownership boundary to escalate across) and that the transition is reversible (claimed → paid is an allowed transition, so a wrongly-swept deposit can be corrected via the existing deposit-status endpoint), I'm treating this as a data-integrity / business-logic gap rather than a privilege-escalation vulnerability. Recommend (non-blocking): additionally constrain sweepEligibleDeposits to deposits whose invoiceId is drawn from a server-derived relevant-invoice set (e.g. intersect with invoices that have a budget line or tagged deposit under sourceId) rather than trusting the client-submitted depositIds linkage as-is.

3. InvoicesNotClaimableError detail exposure

Returns { invoiceIds: offendingIds } — a subset of the same invoice IDs the caller submitted in the request body (or their own report data). No new sensitive-data disclosure; consistent with the existing StaleOwnershipError convention of returning offending IDs in details.

Summary

No injection, auth-bypass, or sensitive-data-leak findings. One Low finding (depositIds sweep scope) and one Informational note (missing size caps on mark-claimed body fields) — neither blocks merge. CI's two failing E2E shards (2/16, 5/16) are outside security review scope.

[steilerDev]

[product-architect]

Architecture review — PR #1922 (#1895 / #1896 / #1918)

Verified against wiki/API-Contract.md @ 2f678b7 (submodule ref correctly bumped on the branch: 905182d → 2f678b7), the dual-rail model, and the intended semantics from the audit that filed #1895/#1896.

Verdict: REQUEST CHANGES — one high finding (client emits a request the server contract rejects). Everything else is medium/low and would not block on its own.

---

🔴 High — invoiceIds: [] is a reachable client request that the contract rejects with 400

ReportWizardPage.handleMarkClaimed (client/src/pages/ReportWizardPage/ReportWizardPage.tsx:437-447) deliberately drops every invoice that has an excluded budget line from invoiceIds, while keeping its deposits in depositIds. That is the correct semantics for #1895. But three layers still enforce the old "invoiceIds is the batch" rule:

• server/src/routes/sourceReports.ts:80 — invoiceIds: { minItems: 1 }
• server/src/services/sourceReportService.ts:545 — if (invoiceIds.length === 0) throw new ValidationError(...)
• wiki/API-Contract.md — "Array of invoice UUIDs. Must have ≥1 ID."

So the moment every included invoice has at least one excluded line — trivially reachable with a single-invoice claim report where the user unchecks one line — the wizard POSTs { sourceId, invoiceIds: [], depositIds: [...] }, gets 400 VALIDATION_ERROR, surfaces the generic sourceReports.claimFailed toast, and the deposit sweep that the fix exists to perform never runs.

This is enshrined rather than caught by the current tests:

• client/src/pages/ReportWizardPage/ReportWizardPage.test.tsx:1551 asserts toHaveBeenCalledWith('src-1', [], ['dep-1']) against a mocked API client — it never touches the schema.
• server/src/routes/sourceReports.test.ts:351 asserts the opposite (invoiceIds: [] → 400).
• The E2E that covers this exact scenario (e2e/tests/budget/reportWizardExpansion.spec.ts:669) asserts the warning copy and then calls wizard.cancelClaimConfirm() — it never submits, so the 400 is invisible end-to-end.

The two unit suites are mutually contradictory and both pass. That is the signature of a contract seam with no integration coverage across it.

Requested fix — the operation's semantics have genuinely changed (invoiceIds = attempt-to-flip set, depositIds = sweep set), so the ≥1 constraint belongs on the union, not on invoiceIds:

1. server/src/routes/sourceReports.ts — drop minItems: 1 from invoiceIds.
2. server/src/services/sourceReportService.ts:545 — replace the empty-invoiceIds guard with if (invoiceIds.length === 0 && depositIds.length === 0) throw new ValidationError(...). The claimability loop, the other-source query (already guarded by if (invoiceIds.length > 0)), and the write loops are all no-ops on an empty invoiceIds, so no other logic changes.
3. wiki/API-Contract.md — update the invoiceIds validation cell to "May be empty when depositIds is non-empty (deposit-only close-out). At least one of invoiceIds / depositIds must be non-empty." and note the 400 condition.
4. Coverage: flip server/src/routes/sourceReports.test.ts:351 to the both-empty case and add a service test for invoiceIds: [], depositIds: ['dep-1'] sweeping successfully; extend the E2E at :669 past cancelClaimConfirm() to actually confirm and assert the deposit flipped while the invoice status did not.

---

🟡 Medium — success count is taken from the client's inclusion set, not the server response

ReportWizardPage.tsx:453 — await markInvoicesClaimed(...) discards MarkClaimedResponse, and setClaimedCount(included.length) counts every non-invoice-excluded invoice. After this PR, invoices routinely don't flip (other-source interest, quotation, excluded lines), so sourceReports.claimSuccess ("{{count}} invoice(s) marked as claimed") over-reports. In the PR's own test case the server returns claimedInvoiceIds: [] while the UI would report 1. Please capture the response and derive the count from claimedInvoiceIds (and consider surfacing claimedDepositIds.length for the deposit-only close-out, which is otherwise silent to the user).

🟡 Medium — sourceId is not checked against an existing budget source

POST /mark-claimed accepts any non-empty string. A stale or bogus sourceId degrades silently into "sweep untagged deposits only, and treat every tagged budget line as other-source interest" — a write that quietly does the wrong thing rather than 404-ing. GET /api/source-reports has the same gap, so this is consistent with precedent rather than a regression, but the write path deserves an existence check (404 RESOURCE_NOT_FOUND). Worth a follow-up issue if you'd rather not widen this PR.

🔵 Low — NULL-tagged budget lines are (correctly) not "other-source interest"

COALESCE(wib.budget_source_id, hib.budget_source_id) IS NOT NULL AND ... != ${sourceId} means an invoice mixing sourceId-tagged lines with untagged lines still flips to claimed. That is right — an untagged line is claimable by no source, so nothing is stranded — but the contract's "no other-source budget line interest" wording doesn't say so. One clarifying sentence in API-Contract.md would prevent a future reader re-litigating it.

---

Verified clean

• Transaction integrity. Every read and write in markInvoicesClaimed goes through tx, including the new raw tx.all(sql\...`). The union fetch (invoiceIds∪ sweep-eligible deposits'invoiceIds, sourceReportService.ts:563-566) is exactly what makes the invoiceById.get(deposit.invoiceId)!non-null assertion in the diary-event path safe now that deposits can belong to invoices outsideinvoiceIds` — good catch by the implementer; the naive version would have thrown.
• Other-source SQL. Parameterized via sql.join(ids.map((id) => sql\${id}`))— bind parameters, no interpolation — and structurally a mirror of the existingisSplitMapquery in the same file, so the pattern is consistent. TheCOALESCEover the twoLEFT JOINs is correct for both work-item and household-item budgets: the invoice_budget_linesXOR CHECK (ADR-018) guarantees exactly one side joins, the other yieldsNULL, and an untagged budget yields NULLon both and is filtered byIS NOT NULL. Unbounded IN` list inherits SQLite's parameter cap, same as the pre-existing query — immaterial at this scale.
• Claimability widening. claimableDirectly || hasClaimableDeposits (dropping the status === 'claimed' && qualifier) admits exactly quotation as the new case, since InvoiceStatus is pending | paid | claimed | quotation — no unintended status is reachable. Matches the contract text and the product owner's Bug: quotation-status invoice with a pending deposit enters the claim report and 409s the entire mark-claimed batch #1896 decision.
• Bug: claim report shows non-applicable budget lines for quotation invoices entering via an unclaimed deposit #1918 filter scoping. type === 'claim' && portion === 0 sits inside the budget-line assembly loop only (sourceReportService.ts:378-381); that loop performs no accumulation, so allocatedAmount, totalAmount, the deposits sub-table, and the budget-overview / proof-of-funds reports are provably unaffected. budgetLines: budgetLinesByInvoiceId.get(invoiceId) ?? [] (:454) keeps an all-zero invoice present with an empty line array rather than dropping the invoice — the intended behavior. Equality against 0 is safe because portion comes out of toCents(...) / 100.
• Types & conventions. MarkClaimedRequest in shared/ matches the route schema and the wiki table field-for-field; snake_case confined to the raw SQL, camelCase everywhere in TS; no new any.

ADR question — confirming the spec decision: no ADR needed

This changes a request shape and refines a behavioral rule inside an already-decided model; it introduces no new technology, no new pattern, and no schema change. ADR-018 (invoice_budget_lines M:N) and the #1891 deposit-tagging work already carry the structural decisions this builds on, and API-Contract.md is the right home for the claimability/sweep rules. Agreed — no ADR amendment.

One durable-knowledge note: "invoice status is source-agnostic, therefore a multi-source invoice never flips on a single-source claim" is a domain invariant that will outlive these three tickets. It currently lives only in the Behavior bullet list. If deposit-only close-out semantics get reused anywhere else, promote it to an ADR then.

[steilerDev]

[product-architect]

Re-review — PR #1922 @ 04d32506

Re-verified all four findings from my earlier review against commits 28ed8be7, 59f18ae9, 04d32506, plus wiki 2f678b7 → d6e13dc (submodule ref correctly bumped on the branch).

Verdict: APPROVE — every finding is resolved, and the high one is resolved at the layer where it actually mattered (real ajv compilation through app.inject, plus a UI-level E2E), not just in mocks. Remaining notes are informational only.

---

✅ HIGH — empty invoiceIds (resolved, correctly)

The fix moves the ≥1 constraint onto the union, which is the right modelling call now that invoiceIds and depositIds are independent sets:

• sourceReports.ts:80-98 — minItems dropped from invoiceIds, replaced by an anyOf requiring one of the two to be non-empty.
• sourceReportService.ts:545 — guard is now invoiceIds.length === 0 && depositIds.length === 0.
• API-Contract.md — new Request Validation Rules section plus a corrected 400 row.

What makes this convincing rather than merely plausible is that the three branches are covered at the route level (sourceReports.test.ts), so the anyOf is exercised by the real Fastify/ajv-compiled validator rather than asserted in the abstract: 400 both-empty, 200 deposit-only with claimedInvoiceIds: [] / claimedDepositIds: [depId], and 400 missing-field. That closes exactly the seam I flagged — the contradiction between the mocked client test and the server schema is gone because both now agree on the same rule.

The client-side claimNothingClaimable short-circuit (ReportWizardPage.tsx:450-455) is a good addition beyond what I asked for: the both-empty case is now a specific, actionable message instead of a round-trip to a generic 400. Belt and braces, in the right order.

E2E Scenario 11 is the piece I most wanted and it lands properly. Unlike the Scenario 6 test that stopped at cancelClaimConfirm(), this one drives the full flow through the UI (expand → exclude the invoice's only line → confirm), then asserts the outcome out of band via the API — invoice still pending, deposit claimed — rather than trusting the banner. It also deliberately picks a shape distinct from Scenario 9's structurally-zero-lines case, so the three deposit-only close-out paths (cross-source funding, zero-line-for-source, manual line exclusion) are each independently covered. The header comment documenting why they differ is the kind of thing that keeps this suite maintainable.

✅ MEDIUM — over-reported success count (resolved)

claimedCount is replaced by claimedInvoiceCount/claimedDepositCount sourced from the response (ReportWizardPage.tsx:456-458), threaded through Step5Actions, with copy reworded to "{{invoices}} invoice(s) and {{deposits}} deposit(s) marked as claimed" in both locales. Surfacing the deposit count was optional in my original note — including it is the better call, because the deposit-only close-out was otherwise completely invisible to the user. The E2E assertions on "0 invoice(s) and 1 deposit(s)" (Scenario 11) and "1 invoice(s) and 1 deposit(s)" (Scenario 10) pin the client-vs-server divergence in place, which is the whole point.

✅ MEDIUM — unvalidated sourceId (resolved) — and a correction to my own review

Step 0 inside the transaction (sourceReportService.ts:554-558) → NotFoundError → 404 NOT_FOUND, with route and service tests. Placement inside the transaction is fine: it's a read, and better-sqlite3 rolls back and rethrows cleanly.

Correction: I wrote that GET /api/source-reports "has the same gap, so this is consistent with precedent." That was wrong — getSourceReport has validated the source since before this PR (sourceReportService.ts:58). So mark-claimed wasn't following a precedent, it was diverging from the sibling endpoint on the same resource, on the write path. The finding was right; my rationale understated it. The fix now makes the two consistent.

✅ LOW — contract wording (resolved)

The new Other-Source Budget Line Interest section states plainly that NULL-attributed budget lines are deliberately not counted. That's the sentence I wanted; it should stop this being re-litigated.

✅ Bonus — dead CSS class

Step5Actions.tsx:133 referenced sharedStyles.formErrorBanner, which does not exist in shared.module.css — the claim error banner has been rendering unstyled. Now bannerError (which does exist, :437) with role="alert", and the E2E POM locator updated to match. Correctly identified as pre-existing and fixed in place rather than worked around; the role="alert" addition also makes it announce to screen readers, which it previously did not.

---

Informational (no action required)

1. Pre-action counts still use the client's inclusion set. The "Mark N invoices as claimed" button label and confirmClaimBody's {{count}} remain report.invoices.length - excludedInvoiceIds.size, so they can over-state what will actually flip. That's defensible for a pre-action estimate — the client genuinely cannot know the other-source outcome before the call — and the post-action banner is now authoritative. Leaving as-is is the right trade.
2. Redundant required inside the anyOf branches. Both fields are already required at the top level, so the branch-level required is noise. Harmless. Also worth knowing for future readers: additionalProperties: false alongside anyOf is safe here only because the subschemas declare no additionalProperties of their own — under Fastify's default removeAdditional: true, subschemas that did would be a known ajv footgun. The three route tests compile the real validator, so this is empirically confirmed, not just reasoned.
3. claimSuccess no longer uses i18next's count variable, so it no longer participates in plural resolution. No behavior change today — both locales use a literal "(s)" / "(en)" — but a two-quantity string can't express real plural forms for both independently. If either locale ever needs true plurals here, it'll need splitting into two keys.

ADR position unchanged

Still no ADR amendment required. The fix rounds added validation rules and an error code, all of which belong in API-Contract.md and are now documented there. My earlier suggestion stands as a note for later, not this PR: "invoice status is source-agnostic, therefore a multi-source invoice never flips on a single-source claim" is a durable domain invariant that deserves promotion to an ADR if deposit-only close-out semantics get reused beyond the report wizard.

Nice work across the three rounds — particularly pushing the verification down to route-level and UI-level tests instead of adjusting the mocks to match.

[github-actions]

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

The release is available on GitHub release

Your semantic-release bot 📦🚀