feat(budget-sources): GET /api/budget-sources/:sourceId/budget-lines (#1245)

[steilerDev]

Summary

• Add GET /api/budget-sources/:sourceId/budget-lines endpoint that returns all budget lines (work item lines + household item lines) linked to a given budget source
• New shared types BudgetSourceBudgetLine and BudgetSourceBudgetLinesResponse expose parentId, parentName, area, and hasClaimedInvoice alongside the existing BaseBudgetLine fields
• Results are sorted by area (nulls last), then parent name, then createdAt
• API-Contract wiki page updated with the new endpoint documentation

Fixes #1245

Test plan

• Unit tests for budgetSourceService new functions (budgetSourceService.test.ts)
• Integration tests for GET /api/budget-sources/:sourceId/budget-lines route (budgetSources.test.ts)
• Pre-commit hook quality gates pass

Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) noreply@anthropic.com

[github-actions]

Thank you for your submission! We require all contributors to sign our Contributor License Agreement before we can accept your contribution.

To sign, please comment on this PR with:
I have read the CLA Document and I hereby sign the CLA

---

I have read the CLA Document and I hereby sign the CLA

---

Frank Steiler seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.}

[steilerDev]

[product-owner] APPROVE — all 7 acceptance criteria from #1245 are met. (Submitting as comment because the PR author is the bot account; treat this as the product-owner approval for the /develop review gate.)

AC Verification

#	AC	Status	Evidence
1	200 with { workItemLines, householdItemLines }, each extends BaseBudgetLine with parentId, parentName, area, hasClaimedInvoice	PASS	shared/src/types/budgetSource.ts — BudgetSourceBudgetLine extends BaseBudgetLine with the four additional fields; BudgetSourceBudgetLinesResponse defines the two arrays. Route test response includes all BaseBudgetLine fields verifies all 19 inherited fields + 4 extension fields.
2	Empty source → both arrays empty	PASS	Route test returns 200 with empty arrays when source has no budget lines; service test returns empty arrays when source has no budget lines.
3	Nonexistent sourceId → 404	PASS	Service throws NotFoundError('Budget source not found'); route test returns 404 for nonexistent sourceId verifies error.code === 'NOT_FOUND'.
4	Unauthenticated → 401	PASS	Route handler throws UnauthorizedError when !request.user; test returns 401 without authentication verifies error.code === 'UNAUTHORIZED'.
5	hasClaimedInvoice: true iff linked invoice status === 'claimed'	PASS	SQL uses CASE WHEN COUNT(CASE WHEN i.status = 'claimed' THEN 1 END) > 0. Covered by positive cases (claimed → true) and negative cases (paid, pending, quotation, no invoice → false) in both service and route tests, for both WI and HI lines.
6	Sort: area name (nulls last) → parent name → createdAt	PASS	compareBudgetSourceLines applies the three-tier sort in exact order. Service tests verify each tier: sorting: work item lines sorted by area name (nulls last), two lines in same area sorted by parentName, two lines on same work item sorted by createdAt.
7	Reuse join helpers from budgetSourceService.ts rather than duplicating	PASS	buildWorkItemBudgetLine / buildHouseholdItemBudgetLine import toAreaSummary, toBudgetCategory, toBudgetSourceSummary, toVendorSummary, toUserSummary from ./shared/converters.js. The previously local toUserSummary helper in budgetSourceService.ts was removed in favor of the shared version — no duplication.

Additional verified quality

• Response shape matches the wiki API-Contract spec
• Member and admin roles both authorized (consistent with other /budget-sources endpoints)
• actualCost / actualCostPaid / invoiceCount correctly derived from invoice_budget_lines joined with invoices
• invoiceLink populated from first linked invoice, null when none
• 24 service unit tests + 12 route integration tests = 36 tests covering shape, auth, sorting, invoice aggregation, entity resolution, cross-source isolation

Observations (non-blocking, future refinement)

• N+1 query pattern in buildWorkItemBudgetLine / buildHouseholdItemBudgetLine: each line issues ~8 sequential point-lookup queries (work/household item, area, category, source, vendor, user, invoice aggregates, invoice link). For sources with 50+ lines this could add up. Consider consolidating into a single LEFT JOIN query in a future refinement — not a blocker for this story.
• JS-layer sort instead of SQL ORDER BY: acceptable given the JS-layer joins, but flag if pagination is later added.

Verdict: APPROVE. Orchestrator may merge once CI gates are green.

[steilerDev]

[security-engineer] PR #1251 — Security Review: GET /api/budget-sources/:sourceId/budget-lines

Authentication

The new route follows the identical if (!request.user) { throw new UnauthorizedError(); } guard used by every other handler in budgetSources.ts. Auth enforcement is correct and consistent.

Test case #1 in budgetSources.budgetLines.test.ts explicitly asserts a 401 with UNAUTHORIZED on unauthenticated requests — verified by the test suite.

Injection

All SQL in the new code paths is parameterized:

• getBudgetSourceBudgetLines: Drizzle eq(budgetSources.id, sourceId) — safe.
• buildWorkItemBudgetLine / buildHouseholdItemBudgetLine: All entity lookups use eq() with ORM table references — safe.
• getWorkItemLineInvoiceData, getHouseholdItemLineInvoiceData, getWorkItemLineInvoiceLink, getHouseholdItemLineInvoiceLink: All use sql`...WHERE ibl.work_item_budget_id = ${lineId}` tagged templates — Drizzle parameterizes these — safe.

No raw string interpolation of user-controlled values anywhere in the new code.

IDOR

sourceId from the URL is used in a WHERE clause without a per-user ownership check. This is consistent with every other budget-source route in the file (list, get-by-id, patch, delete — none filter by user). The application is single-tenant (1–5 homeowners sharing all data by design), so there is no IDOR risk here. No finding.

Information Disclosure

The BudgetSourceBudgetLine shape is built via explicit field mapping in buildWorkItemBudgetLine / buildHouseholdItemBudgetLine. Sensitive fields examined:

• createdBy → toUserSummary(createdByUser) — strips everything except id, displayName, email. No password hash or OIDC subject leaks.
• budgetSource → toBudgetSourceSummary(source) — summary shape, no sensitive fields.
• hasClaimedInvoice — boolean derived from invoice status, not sensitive.

No sensitive data exposure.

Query Volume (Informational)

getBudgetSourceBudgetLines fetches all lines in two bulk SELECTs, then iterates each line synchronously executing up to 8 individual lookups (work item/household item, area, category, source, vendor, user, invoice aggregates, invoice link). For a source with N budget lines this is O(N × 8) round-trips to SQLite.

At the documented scale of 1–5 homeowners this is unlikely to be a practical problem. However, if sources accumulate hundreds of lines (realistic for a full home build), response times could grow noticeably. This mirrors an already-accepted pattern from earlier EPICs. No blocking concern for this PR — noting for future optimization consideration.

Severity: Informational — no exploit path, scale-constrained deployment.

Response Schema (Informational)

budgetLinesParamsSchema defines params but no response schema. This is consistent with other routes in the file (e.g., the list route). No security impact — explicit field mapping in the converter functions is the defense-in-depth layer against accidental field leakage.

Severity: Informational — consistent with existing patterns.

Checklist

• No SQL/command/XSS injection vectors in new code
• Authentication enforced on new endpoint (matches file-wide pattern)
• No sensitive data exposed in logs, errors, or client responses
• User input (sourceId) used only via parameterized queries
• No new dependencies
• No hardcoded credentials or secrets
• IDOR not applicable (single-tenant, shared-data model)

Verdict: No blocking findings. Approve when the token constraint allows.

[steilerDev]

[product-architect]

Review posted as a comment because the orchestrator account authored this PR and GitHub blocks self-approvals. Treat this as an approve verdict; all findings are medium and non-blocking.

Reviewed the new GET /api/budget-sources/:sourceId/budget-lines endpoint for architectural compliance. Scope: route wiring, service design, shared-type cohesion, convention alignment with BaseBudgetLine / BudgetSourceSummary / AreaSummary, query shape, wiki contract.

What passes

• Route shape / conventions: mounted under /api/budget-sources/:sourceId/budget-lines (kebab-case, matches convention), uses fastify-schema params validation, UnauthorizedError guard, returns 200 with grouped arrays, 404 with standard error shape when source is missing. Matches the rest of budgetSources.ts.
• Type design: BudgetSourceBudgetLine extends BaseBudgetLine with only 4 additive fields (parentId, parentName, area, hasClaimedInvoice) — cohesive with existing budget line types. Reuses AreaSummary, BudgetSourceSummary, BudgetLineInvoiceLink. No parallel type hierarchies introduced.
• Wiki contract: API-Contract.md updated in wiki commit 88c2e6d with full typescript signature, JSON example, field notes, sorting rules, and error table. Contract matches the implementation.
• Converter consolidation: The old local toUserSummary in budgetSourceService.ts is removed in favor of shared/converters.ts. Nice cleanup — this is the right pattern.
• Vendor shape: toVendorSummary (without trade resolution) is used consistently with budgetServiceFactory.resolveRelationsBatch for BaseBudgetLine responses. Wiki example JSON shows \"trade\": null, matching what the endpoint emits.
• Sort rules: area name (nulls last) → parentName → createdAt, implemented in compareBudgetSourceLines with explicit null-handling. Documented in wiki and tested.
• Test coverage: 36 tests across service + route cover invoice status variants (claimed / paid / pending / quotation / none), null-area sorting, mixed WI/HI, parent name matching, source isolation, auth (401, member, admin), and field-shape assertions. Comprehensive for a read-only endpoint.
• ADR alignment: Fits ADR-001 (Fastify), ADR-003 (Drizzle + raw sql for aggregates), ADR-007 (npm workspaces / shared types). No new ADR required — this is an additive endpoint within the existing budget-source contract.

Medium findings (non-blocking, flag for refinement)

M1. Duplicated helpers already available in budgetServiceFactory.ts

budgetSourceService.ts defines four new private helpers — getWorkItemLineInvoiceData, getHouseholdItemLineInvoiceData, getWorkItemLineInvoiceLink, getHouseholdItemLineInvoiceLink — whose SQL is nearly identical to existing exports:

• getInvoiceAggregates(db, budgetId, invoiceBudgetIdColumn) — same COALESCE(SUM(...)) + actualCostPaid pattern, differs only in that the new code adds a hasClaimedInvoice column in the same SELECT.
• getInvoiceLink(db, budgetId, invoiceBudgetIdColumn) — identical SQL to the two new getWorkItemLineInvoiceLink / getHouseholdItemLineInvoiceLink helpers.

Consider either (a) extending getInvoiceAggregates in budgetServiceFactory.ts to return hasClaimedInvoice (or add a sibling getInvoiceAggregatesWithClaim) and calling the existing getInvoiceLink, or (b) accept the duplication as intentional scoping and add a short comment explaining why the shared helpers weren't used.

M2. Per-line resolution (N+1 shape)

buildWorkItemBudgetLine / buildHouseholdItemBudgetLine each issue 8 per-line queries (work_item | household_item, area, category, source, vendor, user, invoice aggregates, invoice link). For a source with N lines, this is ~8N queries. The existing resolveRelationsBatch in budgetServiceFactory.ts already implements the batch pattern (one inArray query per dimension + one grouped aggregate query).

At <5 users with typical source sizes this is acceptable and matches the project's simplicity-first principle, but the shape means even a single bulk parent-with-area/category/vendor query would remove most of the lookups. Consider extracting a resolveEntityWithArea helper or extending resolveRelationsBatch to optionally resolve parent + area lookups. Follow-up refactor candidate, not a blocker.

M3. vendor.trade always null on this endpoint (contract note)

The wiki JSON example shows \"vendor\": { \"id\": ..., \"name\": ..., \"trade\": null }. This matches toVendorSummary behavior and is consistent with budgetServiceFactory's BaseBudgetLine output, but it's surprising given that /api/vendors resolves trade. If the frontend for #1247 needs trade badges on source-scoped budget lines, the consumer will need a separate lookup or we'll need to switch to toVendorSummaryWithTrade. Flag for product-owner / UX before #1247 implementation; no change required here.

No findings on

• Error handling (standard shape, correct status codes)
• Response shape (exactly matches wiki)
• Auth / authorization
• SQL injection risk (parameterized via Drizzle sql template)
• Migration impact (none — read-only endpoint)
• ADRs (no new decision required)
• Shared type exports (correctly re-exported from shared/src/index.ts)

Verdict

Approve. M1 and M2 are refactor opportunities, not contract violations. The endpoint is correct, the wiki is updated in-PR, and tests are thorough. Merge unblocked; file follow-up issues against budgetSourceService performance / converter reuse during refinement.