feat(budget): add source contact fields, household settings, and document attachment typing

[steilerDev]

Summary

• Adds reference and contactAddress fields to budget sources for the upcoming Bank Report Wizard
• Adds a household sender settings endpoint/table (app_settings) so the household's own contact details can be recorded
• Adds attachment-type tagging (quotation / deposit / invoice) to document links on invoices, including a PATCH /api/document-links/:id endpoint to tag/retag/clear the type

Fixes #1877

Test plan

• Unit tests pass (95%+ coverage)
• Integration tests pass
• CI Quality Gates pass (typecheck, tests, build, audit)

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

[steilerDev]

[security-engineer] Security review of PR #1883 (Story #1877 — budget-source contact fields, document attachment typing, household settings).

Scope reviewed

• server/src/routes/settings.ts (new GET/PATCH /api/settings) + server/src/services/appSettingsService.ts
• server/src/routes/documentLinks.ts (new PATCH /api/document-links/:id) + server/src/services/documentLinkService.ts
• server/src/routes/budgetSources.ts contact fields (reference, contactAddress)
• Migrations 0041_budget_source_contact_fields.sql, 0042_document_links_attachment_type.sql, 0043_app_settings.sql
• shared/src/types/settings.ts, shared/src/types/document.ts, shared/src/types/budgetSource.ts
• Client rendering of the new fields (ManagePage.tsx, BudgetSourcesPage.tsx, LinkedDocumentCard.tsx)

Authorization — GET/PATCH /api/settings (any authenticated user, no admin gate)

Verified this is correct and consistent with the app's established authorization model, not a gap:

• server/src/plugins/auth.ts enforces a global preValidation hook on all /api/* routes (except the small PUBLIC_ROUTES set) — authentication is mandatory app-wide regardless of the route-level if (!request.user) checks in settings.ts, which are defense-in-depth.
• requireRole('admin') is used sparingly and deliberately — only for backups.ts (data export/restore) and users.ts (user account management). Every other household-scoped resource (work items, budget sources, household item categories/Areas/Trades, document links) is writable by any authenticated member with no per-user ownership check — this is a single-household app (1-5 users) where all members are expected to co-manage shared data.
• The PR's claim of parity with the Areas/Trades tabs (householdItemCategoryRoutes) is accurate — that route also has no requireRole gate.
• Confirmed by an explicit test: server/src/routes/settings.test.ts:296 — "is accessible to a non-admin (member) user — no admin gate" — this was a deliberate, tested design decision, not an oversight.

No change requested here. Member-writable app-wide settings is consistent with the existing trust model for a small self-hosted household app.

PATCH /api/document-links/:id — authorization / IDOR

Any authenticated user can retag any link by ID, with no per-user ownership check tying the mutation to createdBy. This is consistent with the existing POST/GET/DELETE handlers in the same file (none of which check ownership either) — not a new pattern introduced by this PR. Given the shared-household trust model above, this is acceptable and not a finding.

documentLinkService.updateAttachmentType (documentLinkService.ts:271-283) correctly re-derives entityType from the existing DB row (not from client input) and forces attachmentType to null for any non-invoice link server-side — the client cannot force an attachment type onto a work-item/household-item/budget-source/subsidy-program link even if it sends one. Good application-layer enforcement, matches the same normalization already used in createLink.

Input validation

• settings.ts: householdName maxLength 200, householdAddress maxLength 500, additionalProperties: false, minProperties: 1 (rejects empty-body PATCH) — enforced both at AJV schema and redundantly in appSettingsService.updateHouseholdSettings (defense in depth).
• budgetSources.ts: reference maxLength 200, contactAddress maxLength 500, both nullable strings, additionalProperties: false on both create and update schemas.
• documentLinks.ts: attachmentType constrained to enum: ['quotation', 'deposit', 'invoice', null] at the AJV schema layer, and independently backed by a DB CHECK constraint in migration 0042 (CHECK(attachment_type IN ('quotation','deposit','invoice') OR attachment_type IS NULL)) — same defense-in-depth pattern noted in prior PRs (feat(invoices): add deposit refund entries with negative claim adjustments #1880) for other enum columns in this codebase.
• All DB access goes through Drizzle ORM parameterized queries (eq(), .values(), .set()) — no raw SQL string interpolation anywhere in the new code. No SQL injection vector.

Stored XSS / sensitive-data exposure

• householdName/householdAddress and reference/contactAddress are rendered only as plain React JSX text/controlled-input value props (ManagePage.tsx, BudgetSourcesPage.tsx) — no dangerouslySetInnerHTML, innerHTML, or eval anywhere in the touched client files. Standard React auto-escaping applies; no XSS vector in this PR's UI.
• Informational / forward-looking note: the route docblock states this data feeds a future "Bank Report Wizard" (PDF export), which is out of scope for this PR (no PDF consumer exists yet in this diff). When that PDF generation is implemented, flag it for review at that time — if it uses an HTML-to-PDF renderer, householdName/householdAddress/reference/contactAddress must be HTML-escaped before interpolation to avoid stored injection into the generated document. No action needed now; noting for the follow-up story's security review.
• No secrets, tokens, or internal error details are leaked in any new response shape or error path reviewed.

Findings summary

No Critical or High findings. No Medium findings. One informational/forward-looking note (PDF escaping, to be revisited when the Bank Report Wizard PDF export ships).

VERDICT: APPROVED

[steilerDev]

[ux-designer] Reviewed the implementation against the visual spec posted on #1877. Went through each of the four surfaces plus token/a11y/dark-mode/responsive checks.

1. Budget source Reference / Contact address fields

BudgetSourcesPage.tsx matches the spec exactly: same .field/.label/.input/.textarea primitives, maxLength={200}/500, rows={3}, placed Terms → Reference → Contact address → Notes in both create and edit forms, disabled wired to isCreating/isUpdating, edit-form id uses edit-reference-${source.id} as specified. No new CSS, no hardcoded values. ✅

2. ManagePage Household tab

Tab is first in tabList/Tab union as specified, role="tab"/aria-selected pattern matches the other five tabs, HouseholdTab follows the ProfilePage-style single-card/dirty-gated-save pattern (disabled={isSaving || (name === savedName && address === savedAddress)} — exact match to spec). Reuses .card/.cardTitle/.cardDescription/.form/.field/.label/.input/.button/.successBanner/.errorBanner — zero new component-specific CSS beyond a .textarea class added to ManagePage.module.css (this file didn't have one yet; it's copied from the same token set as .input, correctly token-driven, stylelint-clean).

Skeleton lines deviation validated: implementation uses <Skeleton lines={5} /> instead of the spec's lines={3}. Checked — all five other ManagePage tabs (AreasTab, TradesTab, OrientationsTab, BudgetCategoriesTab, HICategoriesTab) use lines={5}. Since Household is now a sixth tab sharing the same tabPanel container, matching the sibling tabs' skeleton height avoids a layout jump when switching tabs while loading — a better call than my ProfilePage-derived lines={3}. Approving this deviation; my spec's lines={3} should have deferred to the ManagePage-local convention rather than the ProfilePage one.

3. Attachment-type badges

tokens.css and Badge.module.css are byte-for-byte matches to the spec: --color-attachment-{quotation,deposit,invoice}-{bg,text} in both light and dark blocks, values identical to the already-shipped --color-source-5/8/6 pairs (quotation↔5, invoice↔6, deposit↔8) — confirmed by diffing against the existing --color-source-* tokens. New classes are .attachmentQuotation/.attachmentDeposit/.attachmentInvoice, and .quotation (the existing invoice-status class, unrelated colors) is untouched — no collision. Light-mode contrast checked: 6.41:1 / 8.41:1 / 8.45:1, all comfortably above WCAG AA 4.5:1. Dark-mode values reuse already-shipped, already-audited --color-source-* dark rgba pairs, consistent with the spec's "no new audit needed" rationale.

4. LinkedDocumentCard select + badge

Matches spec: conditional onAttachmentTypeChange/isUpdatingAttachmentType props (same pattern as onItemize), badge only rendered when link.attachmentType is set (no "untagged" chip), <select> includes the empty "None" option to retag back to null, disabled={isUpdatingAttachmentType} gates the control during the PATCH (not the whole card), focus ring uses box-shadow: var(--shadow-focus) (not outline), 44px touch target added inside the existing @media (max-width: 1023px) block alongside .viewButton/.openLink. getByLabelText tests confirm the sr-only label→select association works.

One finding (medium, non-blocking): the spec said reuse sharedStyles.srOnly from client/src/styles/shared.module.css, but the implementation added a new local .srOnly class to LinkedDocumentCard.module.css instead — a byte-for-byte duplicate of the shared one (shared.module.css's own comment already lists three other components using it: AutoItemizePage, PaperlessInvoiceReviewPage, AutoItemizeLineCard). Not a visual or a11y bug, but it's the exact "shared pattern usage" duplication CLAUDE.md asks reviewers to flag. Recommend swapping to import sharedStyles from '../../styles/shared.module.css' + sharedStyles.srOnly and dropping the local class, in refinement or a follow-up.

5. Picker attachment-type field (LinkedDocumentsSection)

Correctly scoped to entityType === 'invoice' only, positioned between modal header and DocumentBrowser, defaults to null/"None", resets on picker close (closePicker and the "Add Document" button handler both reset pendingAttachmentType), passed through hook.addLink(doc.id, pendingAttachmentType). New .pickerAttachmentTypeRow/.pickerLabel/.attachmentTypeSelect classes are fully token-driven (--spacing-*, --font-size-sm, --color-text-primary, --color-bg-primary, --color-border-strong, --radius-md, --shadow-focus), consistent with the card-level select styling.

Scope deviation (flagged by dev-team-lead) — assessed on merits

InvoicePaperlessPickerModal (the create-invoice document picker) does not get a manual attachment-type dropdown; instead invoiceAutoItemizeService.ts auto-tags the created link attachmentType: 'invoice' server-side. This matches what I recommended in the original spec's 3c scope note — surfacing a 4-option control mid-invoice-creation for what's almost always the same answer would have added friction for no benefit. Agree with this resolution.

Cross-cutting checks

• Token adherence: npx stylelint on all five changed CSS files (Badge.module.css, LinkedDocumentCard.module.css, LinkedDocumentsSection.module.css, ManagePage.module.css, tokens.css) — zero violations.
• Dark mode: all new colors route through Layer 2 semantic tokens with dark-block overrides; no component-local dark overrides added.
• i18n parity: checked en/de for both documents and settings namespaces — no missing keys either direction.
• Responsive/touch targets: 44px minimum enforced correctly inside existing breakpoint blocks; no new breakpoints invented.
• A11y: sr-only label + <select> pairing verified via getByLabelText tests; badge text is never color-only; live-region announcements use aria-live="polite" on a plain div (not doubled up with role="status"); error/success banners use role="alert".

Verdict

Everything is token-compliant, dark-mode correct, accessible, and matches the spec with only one non-blocking finding (the duplicated .srOnly class instead of the shared one) and one already-justified deviation (Skeleton lines={5}) and one already-justified scope resolution (auto-tag vs. picker). None of these rise to accessibility or dark-mode severity.

Verdict: APPROVED

[steilerDev]

[product-owner] Product Owner review of PR #1883 against Story #1877 acceptance criteria.

AC coverage

Budget source contact fields

#	Acceptance criterion	Verdict	Evidence
1	Create/edit form on BudgetSourcesPage offers optional Reference (free text) and Contact address (multiline textarea)	PASS	BudgetSourcesPage.tsx adds both fields to the create form and the inline edit form; reference is <input type="text">, contactAddress is <textarea rows={3}>. Both optional, maxLength 200/500, labelled and disabled during submit.
2	reference / contactAddress persist and are returned by the API; both nullable; existing sources default to null	PASS	Migration 0041 adds both columns with no backfill (0041_..._.test.ts asserts a pre-migration row keeps null). budgetSourceService.toBudgetSource maps both; create/update paths validate length and accept explicit null. Route schemas accept ['string','null'].

Tests: budgetSourceService.test.ts (create/update/clear/exactly-at-limit/over-limit, plus a toBudgetSource null-mapping case), routes/budgetSources.test.ts (201/200/400 paths), BudgetSourcesPage.test.tsx (submits values, submits null when blank, pre-fills on edit, clears to null), and E2E budget-sources.spec.ts (create + reload persistence, edit + persistence, optional-omitted).

Household name & address app setting

#	Acceptance criterion	Verdict	Evidence
3	Settings page lets me set an optional household name and address via the app-settings mechanism	PASS	New first Household tab on the Manage page (HouseholdTab in ManagePage.tsx) with name input + address textarea, save button dirty-gated, success/error banners, Skeleton while loading.
4	Values persist across reload	PASS	GET/PATCH /api/settings backed by appSettingsService over the new app_settings key/value table (migration 0043). Partial updates supported; explicit null clears.

Tests: appSettingsService.test.ts (partial update, clear-to-null, length bounds incl. exactly-200/exactly-500), routes/settings.test.ts (200/400/401), settingsApi.test.ts, ManagePage.test.tsx (loading skeleton, populated load, empty load, load error, save + success banner, tab ordering/?tab=household), and E2E settings-manage.spec.ts (fill → save → reload persistence, clear → reload, dirty-gated save, save-failure banner, all six tabs reachable on every viewport).

I accept the adjudicated placement: household name/address is household-wide reference data, not per-user profile data, so the Manage page is the right home and a dedicated settings page would have been a needless new surface. Noting for the record that the Household tab renders first in the tab list but the page's default landing tab is unchanged (areas) — correct call, since changing the default would have been an unrequested behaviour change for existing deep links.

The AC said "stored via the existing app-settings mechanism". There was no app-scoped mechanism (user_preferences is per-user), so migration 0043 generalises that key/value pattern to app scope. That is the minimum needed to satisfy the AC's intent, it is documented in the migration header and on the wiki Schema page, and I do not consider it scope creep.

Document attachment typing

#	Acceptance criterion	Verdict	Evidence
5	attachment_type column added with no backfill; existing links stay null	PASS	Migration 0042, nullable with CHECK(... IN ('quotation','deposit','invoice') OR IS NULL). 0042_..._.test.ts explicitly asserts a pre-migration row is preserved with null, and that an invalid value is rejected.
6	Invoice detail documents section shows an inline selector (No tag / Quotation / Deposit / Invoice) and a Badge for the current tag	PASS	LinkedDocumentCard.tsx renders the four-option <select> and a shared Badge (new attachmentQuotation/attachmentDeposit/attachmentInvoice variants) only when the tag is non-null. Uses the existing shared Badge component with a variant map — no parallel implementation.
7	Changing the tag persists via PATCH /api/document-links/:id, including retag and untag back to null	PASS	New PATCH route + documentLinkService.updateAttachmentType; useDocumentLinks.updateAttachmentType updates local state. Server tests cover tag / retag / untag / 400 invalid enum / 400 missing body / 404.
8	Paperless link-picker can optionally choose an attachment type on create (default: no tag)	PASS	The invoice detail page's Add Document picker shows the type field only for entityType='invoice', defaults to "No tag", and resets on close and after a successful pick. LinkedDocumentsSection.attachmentType.test.tsx covers all four behaviours.
9	Non-invoice entity types always get attachmentType: null	PASS	Normalised server-side in both createLink and updateAttachmentType (not just hidden in the UI), and the selector/handler are withheld client-side for non-invoice entities. Tests cover work_item and budget_source requesting a type and getting null.

I also accept the adjudicated split on the two pickers. The invoice-creation Paperless picker (InvoicePaperlessPickerModal → auto-itemize commit) deliberately offers no choice: the picked document is the invoice's source document, so asking the user to classify it would be a question with only one correct answer. invoiceAutoItemizeService.commitAutoItemizeCreate hard-sets attachmentType: 'invoice' on the link it creates, which is the right way to encode that — the tag ends up present and correct without a redundant prompt. E2E Scenario 21 asserts exactly this: no picker in the flow, and the resulting invoice shows the "Invoice" badge. The AC's "link-picker" is the invoice detail page's Add Document picker, which did get the optional choice (AC 8).

Scope discipline

No undocumented functionality. Everything in the diff traces to an AC, to the shared Badge/token plumbing those ACs require, or to fixture updates for the new attachmentType field. Wiki (API Contract + Schema) is updated in the same change.

#1876 follow-up check

Confirmed: nothing in this PR touches the invoice amount labels, so the "Effective Amount" vs "Final payment" label alignment I flagged on #1876 is neither fixed nor worsened here. It remains open as future work, as agreed — no action required on this PR.

Non-blocking notes (no fix required before merge)

1. Wiki omission, not a contradiction. The API Contract documents attachmentType on POST /api/document-links and the new PATCH, but does not mention that the auto-itemize commit path auto-tags its link as invoice. Since that behaviour is now a deliberate product rule rather than an implementation detail, it is worth a sentence on the API Contract page (or in the Bank Report Wizard notes) so a future reader does not "fix" it as a missing prompt. Fold into the next wizard story.
2. Label wording. The AC wrote "Reference(s)"; the shipped label is "Reference". Singular reads better and I prefer it — recording the deviation only so the AC and the UI are not read as disagreeing later.
3. Design-system consistency (for ux-designer, not blocking). The three new attachment-type colour pairs in tokens.css are raw hex values in the semantic layer, whereas neighbouring status colours reference primitive palette tokens (var(--color-green-100) etc.). Dark-mode overrides, :focus-visible indicators, and the 44px mobile touch target on the selector are all present and correct.
4. Review coverage. Only the security-engineer review is posted on this PR so far. Architecture and QA sign-off should land before merge per the standing process — that is an orchestrator gate, not an AC gap.

Verdict

All nine acceptance criteria are met, each with unit/integration coverage and E2E coverage at the browser level, including the two adjudicated design decisions. No functional gaps, no display-formatting gaps, no scope creep.

APPROVED

[steilerDev]

[product-architect] Architecture review of PR #1883 (story #1877).

Reviewed: migrations 0041/0042/0043, the new app_settings mechanism, attachmentType semantics, shared types, and wiki accuracy (Schema.md + API-Contract.md). Frontend visuals and E2E churn were out of scope for this pass.

Verified

Migrations

• 0041 — two plain nullable ADD COLUMNs, no backfill, accurate rollback note. Standard additive pattern.
• 0042 — ALTER TABLE ... ADD COLUMN ... CHECK(...) is valid on the SQLite we ship, and I re-verified empirically that (a) the CHECK is enforced on subsequent inserts, (b) pre-existing rows get NULL and satisfy the OR attachment_type IS NULL branch, and (c) the documented DROP COLUMN rollback does work even though the column carries its own inline CHECK. Consistent with the enum-column pattern used in 0033/0040.
• 0043 — key as PK, nullable value, no seed rows. Lazy population is documented in the migration header and in Schema.md.
• All three migration tests derive the pre-migration state dynamically from the real migrations directory rather than hardcoding a file list — that is the right pattern and will not rot as migrations accrue.

app_settings mechanism — endorsed. Exposing a fixed-shape, typed household service/routes rather than a generic key-value API is the correct call: it keeps the contract explicit, keeps validation server-side, and avoids an untyped escape hatch that every future feature would abuse. appSettingsService.upsertRawSetting mirrors preferencesService.upsertPreference's select-then-insert/update shape exactly, so the two mechanisms stay legible as siblings.

attachmentType semantics — normalize-not-reject is applied consistently on both write paths (createLink and updateAttachmentType), the DB CHECK permits null so the app-layer scoping is the single enforcement point, and DocumentLinkWithMetadata extends DocumentLink so list responses carry the field with no extra plumbing. The hardcoded 'invoice' in commitAutoItemizeCreate is correct per ADR-032 — that Paperless document is the invoice being created from it. The sibling path (autoItemize() against an existing invoice) correctly leaves the pre-existing link's type untouched, since that value is user-owned.

Contract & conventions — error shapes, status codes, NOT_FOUND/VALIDATION_ERROR codes, snake_case DB / camelCase TS / kebab-case API naming all conform. Every new or changed production file has a matching test file.

Wiki — updated in the same PR with the submodule ref committed on the branch, which is the discipline I most often have to ask for. The diff also repairs two pre-existing staleness bugs (the documented document_links.entity_type CHECK was missing budget_source and subsidy_program in both Schema.md and API-Contract.md). Good catch.

Findings (none blocking)

M1 — PATCH /api/settings: unknown-field stripping is undocumented, and one test misattributes its own 400.
There is no AJV override in server/src/app.ts, so Fastify's default removeAdditional: true applies and additionalProperties: false strips rather than rejects. Consequence: {"householdName":"x","bogus":1} returns 200 with bogus silently dropped. A body of only unknown fields returns 400 — but via the service's "At least one field must be provided" guard, not via the schema (AJV evaluates minProperties before additionalProperties removal, so that keyword does not catch it either). API-Contract.md already documents this behaviour for PATCH /api/invoice-deposits/:id; please add the equivalent note to the PATCH /api/settings section. Also rename server/src/routes/settings.test.ts:172 — "returns 400 when an unknown property is included (additionalProperties: false)" asserts the right thing for the wrong stated reason, and will lead a future reader to believe unknown fields are rejected.

M2 — Architecture.md has no counterpart entry for the new mechanism.
Architecture.md carries a "User preferences architecture" subsection describing the per-user key-value store. Its new app-scope sibling has no entry, so an agent reading Architecture.md will not discover that app_settings exists and may invent a third mechanism. Schema.md and API-Contract.md cover it well; this is just the missing pointer. I will take this as a follow-up.

M3 — This warrants an ADR.
A new app-scope persistence mechanism that generalizes user_preferences and is explicitly intended to accrue future settings clears this project's own ADR bar comfortably (ADR-022 records a status column; ADR-030 records a nullable FK plus a discriminator). I will write ADR-034 covering: the app_settings key-value table, the deliberate choice of a fixed-shape typed API over a generic key-value endpoint, lazy population, and the no-admin-gate decision below. Follow-up on me, not on this PR.

L1 — updateHouseholdSettings writes two rows without a transaction.
server/src/services/appSettingsService.ts validates up front and then issues two independent upsertRawSetting calls, so a partial write is essentially unreachable in practice. Wrapping both in db.transaction(...) would make that atomicity structural rather than incidental.

L2 — No admin gate on PATCH /api/settings. Any authenticated member can change app-wide household metadata. This is deliberate (asserted at settings.test.ts:296) and correct for a <5-user self-hosted household — flagging only so it lands as a recorded decision. I will capture it in ADR-034.

L3 — const getSettingsSchema = {}; (server/src/routes/settings.ts:19) is an empty schema object; drop it and the { schema: getSettingsSchema } option on the GET route.

Informational

• Endpoint/type naming will age awkwardly. GET/PATCH /api/settings returns { settings: HouseholdSettings } — the route is app-scope-generic while the type is household-specific. The first non-household app setting forces either a type name that no longer describes its contents or a response restructure. /api/settings/household, or a generic AppSettings composed of a household block, would age better. Not worth churn now; I will record the trade-off in ADR-034 so the next person adding a setting chooses deliberately rather than by default.
• No uniqueness on (invoice, attachment_type) — several documents on one invoice may all be tagged invoice. I read this as intentional (multi-page and multi-document invoices are real), but the Bank Report Wizard consumer must be written to handle N documents per type rather than assuming one.
• HouseholdTab always submits both fields, so the partial-update path is exercised by tests only, not by the UI. The endpoint semantics are still right.
• Minor UI nit for whoever touches it next: the save button's disabled check compares the untrimmed input against the trimmed persisted value, so after saving " Foo " the button stays enabled.

Nothing here breaks a contract or introduces architectural debt that needs to be paid before merge. M1 is worth folding in during refinement; M2/M3 are mine.

VERDICT: APPROVED

[github-actions]

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 2.13.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀