feat(balance): securities service + detailed snapshot save (#212) #221

Merged

maximus merged 1 commit from issue-212-service-securities into main 2026-06-10 01:07:49 +00:00

Conversation 1 Commits 1 Files changed 2 +1324 -93

maximus commented 2026-06-06 17:17:07 +00:00

Owner

Copy link

Resolves #212

Stacked on #220 (base branch issue-211-conversion-v16, which itself stacks on #219 -> main). Review/merge in order #219 -> #220 -> this.

Service layer for detailed (per-security) balance accounts

• Securities CRUD: findOrCreateSecurity (UPSERT on normalized UPPER(TRIM) symbol, ON CONFLICT(symbol) DO UPDATE; callable inside the save transaction via an optional executor so a brand-new symbol creates its security then its holding atomically), listSecurities, getSecurity, updateSecurity.
• Transactional detailed save: saveSnapshotAtomic / upsertSnapshotLines -- for a detailed line (one carrying a holdings array), the aggregated balance_snapshot_lines row stores the rounded-cent SUM (qty/price NULL) and its balance_snapshot_holdings are rewritten in the SAME BEGIN/COMMIT: capture lastInsertId, DELETE existing holdings, find-or-create each security, INSERT each holding, recompute value = SUM(holdings) server-side. A holding-insert failure rolls the whole save back (tested). upsertSnapshotLines is now wrapped in an explicit transaction too. Simple / legacy-priced scalar path is byte-for-byte unchanged.
• Rounded-cent invariant (decision 2026-06-03): each holding value Math.round(v*100)/100, summed, compared exactly to the line value -- no float tolerance on the detailed path (avoids N-holding rounding accumulation; covered by an N>=20 test). PRICED_VALUE_TOLERANCE kept for the scalar priced path.
• validateDetailedSnapshot: detailed+holdings => line qty/price NULL AND value === rounded-cent SUM; detailed without holdings => pre-pivot aggregated tolerated. validateLineKindInvariants untouched.
• Service backstop: updateBalanceAccount rejects detailed -> simple with typed account_kind_detailed_has_holdings when holdings exist (UI gating alone is insufficient). Adds kind/detailed_since to the account input + SELECTs.
• Reads: getHoldingsForLatestSnapshot (prefill -- latest snapshot holdings per security, excludes quantity 0), listHoldingsBySnapshotLine (drill-down).
• computeUnrealizedGain: per-holding + aggregated value - book_cost and %; book_cost = 0 OR NULL => % = null (N/A, no divide-by-zero); NULL book_cost excluded from the aggregate and flagged (has_unknown_book_cost).

DO-NOT-TOUCH preserved: getSnapshotTotalsByDate/ByCategoryAndDate/ByVehicleAndDate and computeAccountReturn are unchanged -- aggregations and Modified Dietz still read balance_snapshot_lines.value.

Save-input type shape (for #213/#214 downstream)

SnapshotLineInput gains holdings?: SnapshotHoldingInput[] -- presence of the field (even an empty array) marks the line detailed; undefined keeps the simple/priced scalar path. SnapshotHoldingInput = { symbol, asset_type, currency?, security_name?, quantity, unit_price, value, book_cost?, price_source?, price_fetched_at? } (security referenced by normalized symbol for in-txn find-or-create). An empty holdings array models a detailed account at/just after its pivot with no positions yet.

Gate

npm run build ok . npm test (584 passed) ok . cargo check ok (no Rust change). New tests cover every new function: casing dedup, accept/reject incl. N>=20 holdings, computeUnrealizedGain book_cost=0/NULL, detailed->simple guard, atomic save + rollback.

Generated autonomously by /autopilot run of 2026-06-06

Resolves #212 Stacked on #220 (base branch `issue-211-conversion-v16`, which itself stacks on #219 -> main). Review/merge in order #219 -> #220 -> this. ## Service layer for detailed (per-security) balance accounts - **Securities CRUD**: `findOrCreateSecurity` (UPSERT on normalized `UPPER(TRIM)` symbol, `ON CONFLICT(symbol) DO UPDATE`; callable inside the save transaction via an optional executor so a brand-new symbol creates its security then its holding atomically), `listSecurities`, `getSecurity`, `updateSecurity`. - **Transactional detailed save**: `saveSnapshotAtomic` / `upsertSnapshotLines` -- for a detailed line (one carrying a `holdings` array), the aggregated `balance_snapshot_lines` row stores the rounded-cent SUM (qty/price NULL) and its `balance_snapshot_holdings` are rewritten in the SAME `BEGIN/COMMIT`: capture `lastInsertId`, DELETE existing holdings, find-or-create each security, INSERT each holding, recompute `value = SUM(holdings)` server-side. A holding-insert failure rolls the whole save back (tested). `upsertSnapshotLines` is now wrapped in an explicit transaction too. **Simple / legacy-priced scalar path is byte-for-byte unchanged.** - **Rounded-cent invariant** (decision 2026-06-03): each holding value `Math.round(v*100)/100`, summed, compared **exactly** to the line value -- no float tolerance on the detailed path (avoids N-holding rounding accumulation; covered by an N>=20 test). `PRICED_VALUE_TOLERANCE` kept for the scalar priced path. - **`validateDetailedSnapshot`**: detailed+holdings => line qty/price NULL AND value === rounded-cent SUM; detailed without holdings => pre-pivot aggregated tolerated. `validateLineKindInvariants` untouched. - **Service backstop**: `updateBalanceAccount` rejects `detailed -> simple` with typed `account_kind_detailed_has_holdings` when holdings exist (UI gating alone is insufficient). Adds `kind`/`detailed_since` to the account input + SELECTs. - **Reads**: `getHoldingsForLatestSnapshot` (prefill -- latest snapshot holdings per security, **excludes quantity 0**), `listHoldingsBySnapshotLine` (drill-down). - **`computeUnrealizedGain`**: per-holding + aggregated `value - book_cost` and `%`; `book_cost = 0` OR NULL => % = null (N/A, no divide-by-zero); NULL book_cost excluded from the aggregate and flagged (`has_unknown_book_cost`). **DO-NOT-TOUCH preserved**: `getSnapshotTotalsByDate/ByCategoryAndDate/ByVehicleAndDate` and `computeAccountReturn` are unchanged -- aggregations and Modified Dietz still read `balance_snapshot_lines.value`. ## Save-input type shape (for #213/#214 downstream) `SnapshotLineInput` gains `holdings?: SnapshotHoldingInput[]` -- **presence** of the field (even an empty array) marks the line detailed; `undefined` keeps the simple/priced scalar path. `SnapshotHoldingInput = { symbol, asset_type, currency?, security_name?, quantity, unit_price, value, book_cost?, price_source?, price_fetched_at? }` (security referenced by normalized symbol for in-txn find-or-create). An empty `holdings` array models a detailed account at/just after its pivot with no positions yet. ## Gate `npm run build` ok . `npm test` (584 passed) ok . `cargo check` ok (no Rust change). New tests cover every new function: casing dedup, accept/reject incl. N>=20 holdings, computeUnrealizedGain book_cost=0/NULL, detailed->simple guard, atomic save + rollback. Generated autonomously by /autopilot run of 2026-06-06

maximus added the

status:review

autopilot:pending-human

labels 2026-06-06 17:17:07 +00:00

maximus added 1 commit 2026-06-06 17:17:07 +00:00

feat(balance): securities service + transactional detailed snapshot save (#212) ... 582cf4012d

Service layer for detailed (per-security) balance accounts:

- findOrCreateSecurity (UPSERT on normalized UPPER(TRIM) symbol, callable
  in-txn via an executor), listSecurities, getSecurity, updateSecurity.
- saveSnapshotAtomic / upsertSnapshotLines: a detailed account (line carrying
  a holdings array) writes its aggregated line (value = rounded-cent SUM,
  qty/price NULL) AND its holdings in the SAME BEGIN/COMMIT; the line id is
  captured, existing holdings DELETEd, each security find-or-created and each
  holding INSERTed. A holding-insert failure rolls the whole save back. Simple
  / legacy-priced scalar path is unchanged. upsertSnapshotLines is now wrapped
  in an explicit transaction for the same atomicity.
- validateDetailedSnapshot: detailed+holdings => line qty/price NULL and
  value === rounded-cent SUM(holdings) compared EXACTLY (no float tolerance);
  detailed without holdings => pre-pivot aggregated tolerated.
  validateLineKindInvariants stays byte-for-byte for the scalar path.
- roundToCent helper; detailed path uses per-holding cent rounding then exact
  comparison to avoid N-holding rounding accumulation (decision 2026-06-03).
- Service backstop in updateBalanceAccount: detailed->simple rejected with a
  typed error (account_kind_detailed_has_holdings) when holdings exist; adds
  kind/detailed_since to the account input + SELECT.
- getHoldingsForLatestSnapshot (prefill; excludes quantity-0 positions),
  listHoldingsBySnapshotLine (drill-down).
- computeUnrealizedGain: per-holding and aggregated value - book_cost and %;
  book_cost = 0 OR NULL => gain % null (no divide-by-zero); NULL book_cost
  excluded from the aggregate and flagged.

Existing aggregators (getSnapshotTotalsBy*) and computeAccountReturn untouched.
Unit tests for every new function incl. casing dedup, N>=20 holdings rounding,
book_cost=0/NULL, detailed->simple guard, atomic save + rollback. Existing
upsertSnapshotLines/updateBalanceAccount tests updated for the BEGIN/COMMIT
wrapping and the kind/detailed_since columns.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

maximus commented 2026-06-06 19:38:34 +00:00

Author

Owner

Copy link

Adversarial review — PR #221 (issue #212) · APPROVE ✅

Link 3/9, the riskiest TS PR. Verified the FULL diff (+1324/−93, 2 files) plus the v14/v16 schema, the TS types, and a byte-for-byte diff of every DO-NOT-TOUCH function. Re-ran the gate against the actual head content in an isolated overlay (the local tree is on main and would have given a stale pass): vitest 167/167 green, tsc --noEmit 0 errors once the stacked head types/index.ts is in place.

Transactional integrity — correct

• saveSnapshotAtomic and upsertSnapshotLines both wrap the whole rewrite in one explicit BEGIN … COMMIT, ROLLBACK on any throw with the original error preserved over a rollback failure. The aggregated line INSERT → capture lastInsertId → DELETE FROM balance_snapshot_holdings → find-or-create security → INSERT holding all run on the same executor inside that single transaction.
• findOrCreateSecurity(input, exec) threads the open-txn handle: it issues only INSERT … ON CONFLICT … / SELECT, no nested BEGIN (grep confirms the only BEGINs are in the 3 save funcs + the pre-existing proposeStarterAccounts). A nested begin would have errored — it doesn't.
• Rollback is genuinely proven: the holding INSERT fails test asserts the last call is ROLLBACK and that no COMMIT ever ran (no partial line/holdings).

Rounded-cent invariant — correct

• roundToCent(v)=Math.round(v*100)/100 applied per holding, summed, then roundToCent again, compared exactly (!==) to roundToCent(line.value) — no float ε on the detailed path. insertSnapshotLineWithHoldings recomputes the stored line value server-side from the same rounded SUM, so the line is authoritative regardless of caller arithmetic. Covered at N=21 (21×0.005 ⇒ 0.21 exact; 0.22 rejected) in both validateDetailedSnapshot and saveSnapshotAtomic.
• PRICED_VALUE_TOLERANCE retained only for the untouched scalar priced path.

DO-NOT-TOUCH — byte-for-byte unchanged (verified by diff)

getSnapshotTotalsByDate, getSnapshotTotalsByCategoryAndDate, getSnapshotTotalsByVehicleAndDate, computeAccountReturn (Modified Dietz) and validateLineKindInvariants are identical base↔head. The simple/legacy-priced scalar INSERT path in the save helpers is preserved verbatim.

detailed→simple guard — correct, not bypassable

Service backstop in updateBalanceAccount: counts holdings scoped to the account (JOIN balance_snapshot_lines l … WHERE l.account_id = $1, not global), throws typed account_kind_detailed_has_holdings, fires before the UPDATE. Runs only on the dangerous direction (existing.kind==='detailed' && kind==='simple'); simple→detailed skips the count (test asserts exactly one SELECT). UPDATE params […, kind=$7, detailedSince=$8, id=$9] line up.

book_cost guards — correct

computeUnrealizedGain: book_cost===0 ⇒ gain=value, gain_pct=null; book_cost===null/undefined ⇒ gain=null, gain_pct=null, excluded from total_book_cost/total_gain, flags has_unknown_book_cost. Aggregate total_gain_pct = totalBookCost===0 ? null : …. No divide-by-zero on any path; all four branches tested.

SQL / params / symbol normalization

• Every value parameterized ($1…); the only string-literal SQL is NULL/'manual' constants — no interpolation of user data. Holdings INSERT column list matches balance_holdings_schema.sql exactly (book_cost nullable; qty/price/value NOT NULL, always finite per validation).
• normalizeSecuritySymbol = UPPER(TRIM) matches the v16 SQL UPPER(TRIM(a.symbol)) and the symbol TEXT COLLATE NOCASE UNIQUE (v14); ON CONFLICT(symbol) DO UPDATE … COALESCE(name) preserves id + keeps metadata fresh. Casing-dedup tested (aapl/BtC → same row).
• getHoldingsForLatestSnapshot resolves latest per account via snapshot_date DESC LIMIT 1 over lines that carry holdings, then quantity <> 0 (sold-out positions excluded; sell-then-rebuy reappears). listHoldingsBySnapshotLine joins the security and filters by line.

Tests — genuine, not over-mocked

167/167 pass on head. No .skip/.only/.todo. beforeEach resets mockSelect/mockExecute so the mockResolvedValueOnce sequences don't leak. Coverage spans every new function incl. rejects (empty symbol, bad asset_type, scalar-on-detailed, value mismatch, invalid kind, security_not_found), the in-txn executor threading, atomic save, and rollback. The updated upsertSnapshotLines tests correctly re-index for the new BEGIN/COMMIT framing.

Non-blocking nits (optional)

• findOrCreateSecurity's ON CONFLICT … currency=$3, asset_type=$4 overwrites an existing security's currency/asset_type on every save (only name is COALESCE-guarded). Intentional per the doc comment ("keeps metadata fresh"), but a later save with a different/default currency for an existing symbol would silently rewrite it. Consider COALESCE-guarding currency too, or documenting that last-write-wins is intended. Not a correctness bug for the current single-currency (CAD) model.
• computeUnrealizedGain sums total_value from raw stored values (already cents from roundToCent at write time) — fine; just noting the rounding guarantee lives at the write path, not here.

Verdict: APPROVE. No must-fix issues. Privacy-first preserved (all local SQLite, no network). Stacked correctly on #220→#219; merge in order.

## Adversarial review — PR #221 (issue #212) · **APPROVE** ✅ Link 3/9, the riskiest TS PR. Verified the FULL diff (+1324/−93, 2 files) plus the v14/v16 schema, the TS types, and a byte-for-byte diff of every DO-NOT-TOUCH function. **Re-ran the gate against the actual head content in an isolated overlay** (the local tree is on `main` and would have given a stale pass): `vitest` **167/167 green**, `tsc --noEmit` **0 errors** once the stacked head `types/index.ts` is in place. ### Transactional integrity — correct - `saveSnapshotAtomic` and `upsertSnapshotLines` both wrap the whole rewrite in one explicit `BEGIN … COMMIT`, `ROLLBACK` on any throw with the original error preserved over a rollback failure. The aggregated line INSERT → capture `lastInsertId` → `DELETE FROM balance_snapshot_holdings` → find-or-create security → `INSERT` holding all run on the **same executor** inside that single transaction. - `findOrCreateSecurity(input, exec)` threads the open-txn handle: it issues only `INSERT … ON CONFLICT … / SELECT`, **no nested `BEGIN`** (grep confirms the only BEGINs are in the 3 save funcs + the pre-existing `proposeStarterAccounts`). A nested begin would have errored — it doesn't. - Rollback is genuinely proven: the `holding INSERT fails` test asserts the **last** call is `ROLLBACK` **and** that no `COMMIT` ever ran (no partial line/holdings). ### Rounded-cent invariant — correct - `roundToCent(v)=Math.round(v*100)/100` applied per holding, summed, then `roundToCent` again, compared **exactly** (`!==`) to `roundToCent(line.value)` — no float ε on the detailed path. `insertSnapshotLineWithHoldings` recomputes the stored line value server-side from the same rounded SUM, so the line is authoritative regardless of caller arithmetic. Covered at **N=21** (21×0.005 ⇒ 0.21 exact; 0.22 rejected) in both `validateDetailedSnapshot` and `saveSnapshotAtomic`. - `PRICED_VALUE_TOLERANCE` retained only for the untouched scalar priced path. ### DO-NOT-TOUCH — byte-for-byte unchanged (verified by `diff`) `getSnapshotTotalsByDate`, `getSnapshotTotalsByCategoryAndDate`, `getSnapshotTotalsByVehicleAndDate`, `computeAccountReturn` (Modified Dietz) and `validateLineKindInvariants` are **identical** base↔head. The simple/legacy-priced scalar INSERT path in the save helpers is preserved verbatim. ### detailed→simple guard — correct, not bypassable Service backstop in `updateBalanceAccount`: counts holdings **scoped to the account** (`JOIN balance_snapshot_lines l … WHERE l.account_id = $1`, not global), throws typed `account_kind_detailed_has_holdings`, fires **before** the UPDATE. Runs only on the dangerous direction (`existing.kind==='detailed' && kind==='simple'`); simple→detailed skips the count (test asserts exactly one SELECT). UPDATE params `[…, kind=$7, detailedSince=$8, id=$9]` line up. ### book_cost guards — correct `computeUnrealizedGain`: `book_cost===0` ⇒ `gain=value, gain_pct=null`; `book_cost===null/undefined` ⇒ `gain=null, gain_pct=null`, **excluded** from `total_book_cost`/`total_gain`, flags `has_unknown_book_cost`. Aggregate `total_gain_pct = totalBookCost===0 ? null : …`. No divide-by-zero on any path; all four branches tested. ### SQL / params / symbol normalization - Every value parameterized (`$1…`); the only string-literal SQL is `NULL`/`'manual'` constants — **no interpolation of user data**. Holdings INSERT column list matches `balance_holdings_schema.sql` exactly (book_cost nullable; qty/price/value NOT NULL, always finite per validation). - `normalizeSecuritySymbol = UPPER(TRIM)` matches the v16 SQL `UPPER(TRIM(a.symbol))` and the `symbol TEXT COLLATE NOCASE UNIQUE` (v14); `ON CONFLICT(symbol) DO UPDATE … COALESCE(name)` preserves id + keeps metadata fresh. Casing-dedup tested (`aapl`/`BtC` → same row). - `getHoldingsForLatestSnapshot` resolves latest per account via `snapshot_date DESC LIMIT 1` over lines that carry holdings, then `quantity <> 0` (sold-out positions excluded; sell-then-rebuy reappears). `listHoldingsBySnapshotLine` joins the security and filters by line. ### Tests — genuine, not over-mocked 167/167 pass on head. No `.skip`/`.only`/`.todo`. `beforeEach` resets `mockSelect`/`mockExecute` so the `mockResolvedValueOnce` sequences don't leak. Coverage spans every new function incl. rejects (empty symbol, bad asset_type, scalar-on-detailed, value mismatch, invalid kind, security_not_found), the in-txn executor threading, atomic save, and rollback. The updated `upsertSnapshotLines` tests correctly re-index for the new BEGIN/COMMIT framing. ### Non-blocking nits (optional) - `findOrCreateSecurity`'s `ON CONFLICT … currency=$3, asset_type=$4` **overwrites** an existing security's currency/asset_type on every save (only `name` is COALESCE-guarded). Intentional per the doc comment ("keeps metadata fresh"), but a later save with a different/default currency for an existing symbol would silently rewrite it. Consider COALESCE-guarding currency too, or documenting that last-write-wins is intended. Not a correctness bug for the current single-currency (CAD) model. - `computeUnrealizedGain` sums `total_value` from raw stored values (already cents from `roundToCent` at write time) — fine; just noting the rounding guarantee lives at the write path, not here. **Verdict: APPROVE.** No must-fix issues. Privacy-first preserved (all local SQLite, no network). Stacked correctly on #220→#219; merge in order.

maximus changed target branch from issue-211-conversion-v16 to main 2026-06-10 01:07:48 +00:00

maximus merged commit 205e1ded54 into main 2026-06-10 01:07:49 +00:00

maximus referenced this pull request from a commit 2026-06-10 01:07:50 +00:00

Merge pull request 'feat(balance): securities service + detailed snapshot save (#212)' (#221) from issue-212-service-securities into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

autopilot:pending-human

PR produced by /autopilot — awaits human review/merge

  

source:analyste

Créée par le Super-Analyste

  

source:defenseur

Créée par l Escouade Défenseur

  

source:human

Créée par un humain

  

source:medic

Créée par le Medic (auto-détection)

  

status:approved

PR approuvée, prête à merger

  

status:blocked

Bloquée (question, dépendance, clarification)

  

status:in-progress

Un agent (Medic/Dev) travaille dessus

  

status:needs-clarification

Issue blocked by ambiguity — Max needs to clarify before re-running autopilot

  

status:needs-fix

Reviewer demande des corrections

  

status:ready

Analysée et documentée, prête à prendre

  

status:review

PR créée, en attente du Reviewer

  

status:triage

Nouvelle issue, pas encore analysée

  

type:bug

Correction de bug → Medic

  

type:feature

Nouvelle fonctionnalité → Dev

  

type:infra

Config, CI/CD, déploiement → Dev

  

type:refactor

Refactoring / amélioration technique → Dev

  

type:schema

Modèle de données / migrations → Dev

  

type:security

Issue de sécurité → Medic ou Dev

No labels

autopilot:pending-human

source:analyste

source:defenseur

source:human

source:medic

status:approved

status:blocked

status:in-progress

status:needs-clarification

status:needs-fix

status:ready

status:review

status:triage

type:bug

type:feature

type:infra

type:refactor

type:schema

type:security

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: maximus/Simpl-Resultat#221

No description provided.