feat(budget-sources): PATCH /api/budget-sources/:sourceId/budget-lines/move (#1246)

[steilerDev]

Summary

• Add PATCH /api/budget-sources/:sourceId/budget-lines/move endpoint to atomically move budget lines between sources
• Accepts arrays of workItemBudgetIds and/or householdItemBudgetIds plus a targetSourceId; returns arrays of moved lines
• New error codes: SAME_SOURCE (400), EMPTY_SELECTION (400), STALE_OWNERSHIP (409); fully atomic transaction with ownership validation before any UPDATE

Fixes #1246

Test plan

• 16 service-layer unit tests: validation order, atomic rollback on stale IDs, updatedAt bump
• 14 route integration tests: 401, 400 schema/SAME_SOURCE/EMPTY_SELECTION, 404 source-not-found, 409 STALE_OWNERSHIP with DB-unchanged verification, 200 WIB-only / HIB-only / mixed
• Unit tests pass (95%+ coverage)
• Integration tests pass
• Pre-commit hook quality gates pass

Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) noreply@anthropic.com
Co-Authored-By: Claude backend-developer (Haiku) noreply@anthropic.com
Co-Authored-By: Claude qa-integration-tester (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]

[security-engineer]

No security findings. This implementation is clean.

Checklist:

• Auth: if (!request.user) throw new UnauthorizedError() — matches existing pattern
• Injection: all SQL via Drizzle inArray(col, ids) / eq(col, id) — no raw ID interpolation, no string concatenation
• IDOR: both sourceId and targetSourceId are unrestricted — consistent with the established single-tenant model (1–5 homeowners per instance)
• Input validation: JSON schema enforces array-of-string body, all three fields required, targetSourceId: minLength:1; additionalProperties: false strips unknown fields (Fastify removes rather than rejects per established codebase behavior, confirmed by test)
• Transaction integrity: ownership check + UPDATE both run inside db.transaction(tx => {...}) — StaleOwnershipError thrown inside the callback triggers automatic rollback; tests 9, 13, and 14 explicitly verify no DB mutation on rollback
• Return value: { movedWorkItemLines, movedHouseholdItemLines } — counts only, no row data, no sensitive fields leaked
• Error messages: all three new errors use generic messages with no internal detail (source IDs, counts, or row contents)
• Bulk DoS: two independent inArray queries (one per table) — O(N) single-pass, no N+1 loop. No maxItems cap on arrays, consistent with all prior bulk endpoints in this codebase and acceptable at the 1–5 user scale.
• New error codes (SAME_SOURCE, EMPTY_SELECTION, STALE_OWNERSHIP) added to shared errors.ts and matched 1:1 to new AppError subclasses
• Dependency changes: none

[steilerDev]

[product-owner] APPROVED — all 7 ACs from #1246 verified. (Posted as comment: GitHub blocks --approve on self-authored PRs.)

AC Coverage

AC	Code	Tests	Status
1. Body shape + 200 {movedWorkItemLines, movedHouseholdItemLines} + DB updated	MoveBudgetLinesRequest/Response + tx.update	svc #1-4, route #8-10 (explicit getWibSourceId/getHibSourceId DB checks)	PASS
2. 400 SAME_SOURCE when target === URL source	SameSourceError (code SAME_SOURCE, 400)	svc #6/#6b, route #4	PASS
3. 400 EMPTY_SELECTION both arrays empty	EmptySelectionError	svc #5, route #3	PASS
4. 404 target AND source missing	NotFoundError on both lookups	svc #7/#8, route #5/#6	PASS
5. 401 unauthenticated	UnauthorizedError() guard in handler	route #1	PASS
6. 409 STALE_OWNERSHIP — missing OR wrong source — atomic rollback	foundWib.length !== ids.length + row.budgetSourceId !== sourceId, both WIB and HIB paths	svc #9-14, route #7 (all verify DB unchanged post-throw)	PASS
7. Single transaction	Entire validation + update wrapped in db.transaction((tx) => {...})	svc #13 (partial valid + invalid rolls back), svc #14 (WIB valid + HIB wrong source → WIB NOT moved)	PASS

Strengths

• inArray() for efficient batch ownership validation (no N+1)
• Explicit updatedAt bump verified (svc #15)
• Mixed-array atomic rollback tested across WIB/HIB boundary (svc #14)
• Route tests verify DB state post-409 confirming rollback at route layer too
• additionalProperties: false on body schema (Fastify strip behavior verified)
• Member role verified, not just admin

Observations (non-blocking)

• Validation order: source-exists → SAME_SOURCE → target-exists → EMPTY_SELECTION → ownership. AC does not prescribe order; all error codes return correctly. Service test 6b explicitly locks SAME_SOURCE-before-target-lookup ordering.
• New error codes SAME_SOURCE/EMPTY_SELECTION/STALE_OWNERSHIP added to shared ErrorCode union — consistent with project convention.

30 test cases (16 service + 14 route) provide thorough coverage. Ready for merge once CI passes.

[steilerDev]

[product-architect] Architectural review — VERDICT: APPROVE (posted as comment because GitHub blocks self-approval on this PR).

Verified the implementation against the wiki API contract, ADR-001/003/005/007 constraints, and issue #1246 acceptance criteria.

Verified

• Validation order: sourceId exists → SAME_SOURCE → target exists → EMPTY_SELECTION → STALE_OWNERSHIP (AC-compliant). Test 6b explicitly pins the order by passing a nonexistent WIB with targetSourceId === sourceId and asserting SAME_SOURCE wins over any target lookup. Test 13 pins atomic rollback with 2 valid + 1 invalid WIB. Test 14 pins the cross-array rollback (valid WIB + wrong-source HIB → WIB stays put).
• Transaction correctness: db.transaction((tx) => { ... }) wraps both SELECT validations and both UPDATE statements. Throwing any AppError inside the callback triggers rollback — confirmed by tests 9, 10, 13, 14.
• inArray safety: Guarded by explicit if (workItemBudgetIds.length > 0) / if (householdItemBudgetIds.length > 0) checks. Never invoked with an empty array.
• Error handler wiring: errorHandlerPlugin routes all AppError subclasses via error.code + error.statusCode. The three new subclasses (SameSourceError 400, EmptySelectionError 400, StaleOwnershipError 409) match the wiki table at API-Contract.md:4119-4127.
• Wiki accuracy: API-Contract.md documents the validation order (lines 4110-4117), error-response table (lines 4119-4127), and atomic-rollback + two-empty-arrays semantics (lines 4129-4133). Submodule ref bumped from e472e6a to 78aea7d.
• Shared types: ErrorCode union extended with SAME_SOURCE | EMPTY_SELECTION | STALE_OWNERSHIP. MoveBudgetLinesRequest / MoveBudgetLinesResponse exported via shared/src/index.ts. Naming (PascalCase type / camelCase members / snake_case DB columns) follows conventions.
• Test realism: Real SQLite + full migration stack via buildApp(); route tests use app.inject() per ADR-005. Both suites use isolated temp directories per test.
• ADR compliance: ADR-001 (Fastify request.user / reply.status), ADR-003 (Drizzle ORM, better-sqlite3, db.transaction()), ADR-005 (Jest + app.inject()), ADR-007 (@cornerstone/shared types consumed through workspace alias). Endpoint is kebab-case under /api/. Error shape matches wiki contract.
• Scale/perf: Two bulk SELECTs + two bulk UPDATEs. Linear in input size; no N+1.

Minor observations (non-blocking, auto-fix bot territory)

• Low — Prettier drift: a handful of lines exceed printWidth: 100 — notably server/src/routes/budgetSources.ts:222, and several function signatures in server/src/services/budgetSourceService.ts (lines 543, 576, 673, 725, 726) that were collapsed onto a single line. npx prettier --check reports both files as needing reformatting. The auto-fix bot (.github/workflows/auto-fix.yml) handles this on beta push per CLAUDE.md "Local Validation Policy". Not a blocker, but an easy follow-up if the implementing agent wants to normalise before merge.
• Informational — The service does not re-validate Array.isArray(workItemBudgetIds) because the Fastify JSON schema guarantees type: 'array' at the route layer. Acceptable given route-level validation precedes service entry.

Verdict

No critical or high findings. This PR is architecturally sound and contract-compliant. Approve.