feat(balance): convert priced accounts to detailed (v16) (#211) #220
No reviewers
Labels
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
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: maximus/Simpl-Resultat#220
Loading…
Reference in a new issue
No description provided.
Delete branch "issue-211-conversion-v16"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Resolves #211
Stacked on #219 (base
issue-210-schema-migrations).What
Migration v16 — pure data conversion (additive, guarded, atomic) for the Bilan détail-par-titre stack (Étape 2). Converts each existing single-security "priced" account into a
detailedaccount holding exactly one position, with zero data loss.balance_securitiesper priced account symbol (normalizedUPPER(TRIM), deduped viaON CONFLICT(symbol) DO NOTHINGon theCOLLATE NOCASEUNIQUE), only for accounts whose category has a realasset_type(balance_securities.asset_typeisNOT NULL— a priced account whose category hasasset_type IS NULLhas no valid routing).book_coststays NULL — no retroactive acquisition cost).UNIQUE(snapshot_line_id, security_id)+ON CONFLICT DO NOTHING⇒ re-run is a strict no-op.asset_type— is never NULLed ⇒ no silent loss). NULLing both columns together preserves the lines'(both NULL | both NOT NULL)CHECK.A trailing TEMP-table
CHECK(ok = 1)asserts the invariant "qty NULLed ⇒ has a holding" and ABORTS on breach, rolling back the whole v16 transaction (sqlx wraps each migration in its own transaction). Priced accounts withoutasset_typeor without asymbolare left fully intact.Tests (cargo, in-memory SQLite, v10→v16 via execute_batch — mirrors #210 style)
asset_typeaccount untouched (qty intact, no holding)Quality gate
cargo check✅ ·cargo test✅ (94) ·npm run build✅ ·npm test✅ (552)Notes
BEGIN/COMMITinside the migration SQL: sqlx 0.8.6 (tauri-plugin-sql 2.3.2) already wraps each migration in a transaction and rolls back on error, so the guard abort is sufficient. The rollback test reproduces this with an explicit rusqlite transaction dropped on error (SQLite ABORT alone only undoes the offending statement).Generated autonomously by /autopilot run of 2026-06-06
v16 is a purely additive, guarded, atomic data migration (Bilan détail par titre, Étape 2). It converts each existing single-security priced account into a detailed account holding exactly one position, with zero data loss. 1. Mints one shared balance_securities row per priced account symbol (normalized UPPER(TRIM), deduped via ON CONFLICT(symbol) DO NOTHING on the COLLATE NOCASE UNIQUE), ONLY for accounts whose category carries a real asset_type — balance_securities.asset_type is NOT NULL, and a priced account whose category has asset_type IS NULL has no valid routing. 2. Mirrors each existing priced line into one holding (qty/unit_price/value/ price_source/price_fetched_at copied; book_cost stays NULL — no retroactive acquisition cost). UNIQUE(line, security) + ON CONFLICT DO NOTHING makes a re-run a strict no-op. 3. Collapses the now-redundant per-line qty/unit_price to NULL ONLY where a holding now exists (the security fix — a line that got no holding, i.e. priced-without-asset_type, is never NULLed, so no silent data loss). NULLing both columns together preserves the lines' (both NULL | both NOT NULL) CHECK. A trailing TEMP-table CHECK(ok = 1) asserts the invariant 'qty NULLed ⇒ has a holding' and ABORTS on breach, rolling back the whole v16 transaction (sqlx wraps each migration in a transaction). Priced accounts without asset_type or without a symbol are left fully intact. Integration tests (in-memory SQLite, apply v10→v16 via execute_batch, mirroring the #210 migration-test style): convertible account gains a security + holding with values/history preserved and its line qty NULLed; non-convertible priced account untouched (qty intact, no holding); re-run idempotent; injected-failure mid-v16 aborts on the guard and a transaction rollback restores the pre-v16 state (zero securities/holdings, quantity intact). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>Adversarial review — PR #220 (v16 priced→detailed conversion)
Verdict: 🔴 REQUEST_CHANGES — one must-fix (startup-bricking guard false-positive). The conversion logic and atomicity are otherwise correct and well-reasoned; the data-loss guard is sound on the priced-account axis but over-broad on its trigger predicate.
🔴 MUST-FIX — guard false-positive bricks app startup for simple accounts carrying a symbol
src-tauri/src/lib.rs:309(guardWHERE), mirrored in test constV16_SQL.The abort guard keys on
a.symbol IS NOT NULLrather than on convertibility (c.asset_type IS NOT NULL). That makes it fire on a state v16 is not responsible for:A simple-category account that carries a non-NULL
symbol. Its snapshot lines havequantity IS NULLby construction —saveSnapshotforces simple-kind lines to NULL qty/unit_price (balance.service.ts~L899-928, comment L703), and the lines CHECK enforces it. Trace through v16 for such a line:c.asset_type IS NULL) → no security. ✅sl.quantity IS NULL) → no holding. ✅quantity IS NOT NULLpredicate) → not NULLed, no data loss. ✅a.symbol IS NOT NULL✅AND sl.quantity IS NULL✅AND NOT EXISTS (holding)✅ → inserts0→CHECK(ok = 1)fails → the whole v16 migration aborts and rolls back → the app fails to launch for that profile.Reachability is real, via the normal UI (not just the raw service):
AccountForm.tsx:232-249renders the symbol<input>unconditionally;isPricedonly swaps the placeholder/hint. On save (AccountForm.tsx:158) it sendssymbol: trimmedSymbol || nullregardless of kind.updateBalanceAccount(balance.service.ts:451-456) preserves the existing symbol when re-categorizing. So: create a priced accountAAPL, then move it to a simple category (cash) → symbol persists, account is now simple, its lines are qty-NULL → guard trips on next launch.createBalanceAccountL409 /updateBalanceAccountL451) nor any DB CHECK gatessymbolby category kind — "simple ⇒ symbol NULL" is convention only.No data is lost (sqlx rolls back cleanly — verified below), but the user cannot start the app to clear the symbol, so this is a hard startup brick.
Fix: scope the guard's invariant to lines v16 actually owns — i.e. convertible accounts only. Join
balance_categories c ON c.id = a.balance_category_idin the guard subquery and addAND c.asset_type IS NOT NULL. (Equivalently: a line should only be expected to have a holding when its account's category has a real asset_type and the line had a quantity.) Apply the identical change to the productionMigration { version: 16 }SQL and theV16_SQLtest constant.Test gap that hid this:
db_pre_v16()seeds only priced accounts (bothacc_aandacc_b). Add a third account — simple category (cash), non-NULLsymbol, a qty-NULL snapshot line — and assert v16 applies cleanly (guard does not abort) and that line is left fully intact.✅ Verified correct (the hard parts)
id IN (holdings). A holding exists (step 2) only if a matching security exists (step 1 ⇒c.asset_type IS NOT NULL) andsl.quantity IS NOT NULL. So a priced-without-asset_typeaccount mints no security → no holding → step 3 never NULLs it → qty/price retained. Confirmed against schema:balance_securities.asset_typeisNOT NULL CHECK(IN('stock','crypto'))(v14),balance_categories.asset_typeis nullable (v10, set only on seedstock/crypto).UPPER(TRIM(a.symbol))is byte-identical in step 1 and step 2's JOIN; dedupe viaON CONFLICT(symbol)relies on v14symbol TEXT NOT NULL COLLATE NOCASE UNIQUE— present and correct.quantityandunit_pricetogether →(both NULL | both NOT NULL)CHECK onbalance_snapshot_linesupheld.sl.quantity IS NOT NULL⇒ (lines CHECK)unit_price NOT NULL;balance_snapshot_lines.valueisNOT NULLalways →holdings.quantity/unit_price/value (NOT NULL)satisfied.book_costleft NULL (nullable) — fine.execute()returnsErr→ migration rolls back. Verified at source:sqlx-sqlite 0.8.6Sqlite::apply()doeslet mut tx = self.begin().await?; tx.execute(&migration.sql)…?; … tx.commit()with nono_txbranch — every migration runs in a transaction, rolled back on any error. So the production SQL correctly omits BEGIN/COMMIT, and the guard runs after step 3. The rollback test's rusqliteconn.transaction()dropped-without-commit is a faithful proxy (SQLite ABRT alone would only undo the offending statement; the enclosing tx is what guarantees whole-migration atomicity — the test's framing is accurate, not self-deluding).ON CONFLICT(symbol) DO NOTHING+ON CONFLICT(snapshot_line_id, security_id) DO NOTHINGno-op; step 3'squantity IS NOT NULLmeans already-NULLed lines are skipped; guard re-passes (every NULLed line still has its holding). Confirmed bymigration_v16_is_idempotent_on_rerun.value; the mirror holding carries qty/price/value verbatim (migration_v16_converts…asserts both).+393/-0, single file); v1–v15 untouched (checksum-safe); singleversion: 16; consolidated_schema/CHANGELOG/i18n correctly deferred (v16 is UI-invisible).db_through_v13+V14_SQL/V15_SQLhelpers exist and the v9→v15 chain is applied before seeding, so all columns are present.Minor (non-blocking, optional)
asset_type(e.g.ETHasstockin one,cryptoin another) → step 1SELECT DISTINCT … ON CONFLICT(symbol) DO NOTHINGkeeps the first-inserted asset_type (order non-deterministic); step 2 attaches both lines' holdings to that one security. No data loss (qty/price/value preserved), but a possible mis-typing. This is inherent to the v14 single-symbolcatalogue design (one security per symbol), not a v16 regression — flagging for awareness. A typed account sharing a symbol with an untyped-category account will also convert the untyped one (it gets a holding via the shared security); again no loss, arguably desirable.Once the guard predicate is scoped to convertible accounts (and a simple-account-with-symbol test is added), this is a clean APPROVE.
Disposition (décision Max) : merge + fix-forward.
La review a relevé un vrai bug (garde d'abort v16 trop large → blocage de lancement pour un compte simple à symbole résiduel). La logique de conversion elle-même est correcte. Plutôt que de réécrire la chaîne stackée, le fix est différé en fix-forward sur
mainaprès le merge de la chaîne et avant le tag de release — v16 n'a jamais été appliquée à un profil persistant, donc corriger sa SQL avant sa première application respecte la règle d'immutabilité des migrations.Fix tracké dans #228 (type:bug, status:ready, avec SQL + test de régression). Label de l'issue #211 repassé à
status:approvedpour débloquer le merge de la chaîne.Autopilot review — 2026-06-06.