fix: address account valuation review findings

2026-10-06 14:43:29 +00:00
parent 399d0923be
commit 6a8a27a74e
34 changed files with 1940 additions and 131 deletions
@@ -6,7 +6,7 @@ import re
 from typing import Any
 import sqlite3

-from .shared import db, fail, new_id, normalize_date, now_iso, ok, row_to_dict
+from .shared import db, read_db, read_operation, fail, new_id, normalize_date, now_iso, ok, row_to_dict

 _VALID_STATUSES = {"verified", "unverified", "rejected"}
 _VALID_LOAN_DEFINITIONS = {"principal_only", "principal_interest_fees"}
@@ -173,8 +173,11 @@ def register_account_valuation(
                return fail("validation_error", "as_of_at must include timezone offset")
        except (TypeError, ValueError) as exc:
            return fail("validation_error", f"invalid as_of_at: {exc}")
+    if supersedes_id and not correction_reason.strip():
+        return fail("validation_error", "correction_reason is required")
    ts = now_iso()
-    values = (amount_int, currency_norm, (valuation_kind or "balance").strip().lower(), source_norm, source_ref or "", status, memo or "", as_of_at, timezone, optional_ints["cost_basis"], optional_ints["net_contributed"], reported_principal_kind, reported_principal_label, optional_ints["profit"], return_rate, return_basis, optional_ints["principal"], optional_ints["interest"], optional_ints["fee"], definition)
+    source_ref_norm = source_ref.strip() if isinstance(source_ref, str) else ""
+    values = (amount_int, currency_norm, (valuation_kind or "balance").strip().lower(), source_norm, source_ref_norm, status, memo or "", as_of_at, timezone, optional_ints["cost_basis"], optional_ints["net_contributed"], reported_principal_kind, reported_principal_label, optional_ints["profit"], return_rate, return_basis, optional_ints["principal"], optional_ints["interest"], optional_ints["fee"], definition)
    with db(db_path) as conn:
        conn.execute("BEGIN IMMEDIATE")
        account = conn.execute("SELECT * FROM accounts WHERE id=?", (account_id,)).fetchone()
@@ -186,16 +189,47 @@ def register_account_valuation(
            return fail("validation_error", "loan valuation kind requires a loan account")
        if definition is not None and str(account["type"]).lower() != "loan":
            return fail("validation_error", "representative liability definition requires a loan account")
-        if idempotency_key:
-            existing = conn.execute("SELECT * FROM account_valuations WHERE account_id=? AND idempotency_key=?", (account_id, idempotency_key)).fetchone()
-            if existing:
-                current = _valuation(existing)
-                incoming = dict(zip(_COLUMNS, values))
-                incoming.update({"account_id": account_id, "basis_date": basis, "supersedes_id": supersedes_id, "correction_reason": correction_reason or ""})
-                comparable = set(incoming) - {"id", "recorded_at"}
-                if all(current.get(k) == v for k, v in incoming.items() if k in comparable):
-                    return ok(valuation=current, reused=True)
-                return fail("idempotency_conflict", "idempotency_key already has a different payload", valuation=current)
+        seen: set[str] = set()
+        if supersedes_id:
+            parent = conn.execute("SELECT * FROM account_valuations WHERE id=?", (supersedes_id,)).fetchone()
+            if parent is None:
+                if idempotency_key and conn.execute("SELECT 1 FROM account_valuations WHERE account_id=? AND idempotency_key=?", (account_id, idempotency_key)).fetchone():
+                    return fail("idempotency_conflict", "idempotency_key already has a different payload")
+                return fail("validation_error", "supersedes_id must reference an existing valuation")
+            if any(parent[field] != value for field, value in (
+                ("account_id", account_id), ("basis_date", basis),
+                ("currency", currency_norm), ("valuation_kind", (valuation_kind or "balance").strip().lower()),
+            )):
+                return fail("validation_error", "superseded valuation scope must match account, basis, currency, and valuation kind")
+            seen: set[str] = set()
+            cursor = parent
+            while cursor is not None:
+                cursor_id = cursor["id"]
+                if cursor_id in seen:
+                    return fail("validation_error", "supersedes ancestry is cyclic")
+                seen.add(cursor_id)
+                next_id = cursor["supersedes_id"]
+                cursor = conn.execute("SELECT * FROM account_valuations WHERE id=?", (next_id,)).fetchone() if next_id else None
+        existing = None
+        by_key = conn.execute("SELECT * FROM account_valuations WHERE account_id=? AND idempotency_key=?", (account_id, idempotency_key)).fetchone() if idempotency_key else None
+        source_rows = conn.execute("SELECT * FROM account_valuations WHERE account_id=? AND source_ref=? ORDER BY recorded_at, id", (account_id, source_ref_norm)).fetchall() if source_ref_norm else []
+        correction_chain = seen if supersedes_id else set()
+        unrelated_source = next((row for row in source_rows if row["id"] not in correction_chain and (by_key is None or row["id"] != by_key["id"])), None)
+        if by_key is not None and unrelated_source is not None:
+            return fail("idempotency_conflict", "idempotency_key and source_ref identify different candidates")
+        if unrelated_source is not None:
+            by_source = unrelated_source
+        else:
+            by_source = None
+        existing = by_key or by_source
+        if existing:
+            current = _valuation(existing)
+            incoming = dict(zip(_COLUMNS, values))
+            incoming.update({"account_id": account_id, "basis_date": basis, "supersedes_id": supersedes_id, "correction_reason": correction_reason or ""})
+            comparable = set(incoming) - {"id", "recorded_at"}
+            if all(current.get(k) == v for k, v in incoming.items() if k in comparable):
+                return ok(valuation=current, reused=True)
+            return fail("idempotency_conflict", "idempotency_key already has a different payload", valuation=current)
        valuation_id = new_id("valuation")
        try:
            conn.execute("""INSERT INTO account_valuations
@@ -211,20 +245,22 @@ def register_account_valuation(
    return ok(valuation=_valuation(row), reused=False)


+@read_operation
 def list_account_valuations(account_id: str, include_rejected: bool = True, db_path: str | None = None) -> dict[str, Any]:
    where = "" if include_rejected else " AND verification_status <> 'rejected'"
-    with db(db_path) as conn:
+    with read_db(db_path) as conn:
        rows = conn.execute(f"SELECT * FROM account_valuations WHERE account_id=?{where} ORDER BY basis_date, recorded_at, id", (account_id,)).fetchall()
    values = [_valuation(row) for row in rows]
    return ok(account_id=account_id, valuations=values, count=len(values))


+@read_operation
 def select_account_valuation(account_id: str, basis_date: str, as_of: bool = False, currency: str | None = None, valuation_kind: str | None = None, db_path: str | None = None) -> dict[str, Any]:
    try:
        basis = normalize_date(basis_date)
    except Exception as exc:
        return fail("validation_error", str(exc))
-    with db(db_path) as conn:
+    with read_db(db_path) as conn:
        clauses = ["account_id=?"]
        params: list[Any] = [account_id]
        if currency:
@@ -271,6 +307,10 @@ def correct_account_valuation(valuation_id: str, amount: int, correction_reason:
        source = conn.execute("SELECT * FROM account_valuations WHERE id=?", (valuation_id,)).fetchone()
        if source is None:
            return fail("not_found", "valuation not found")
+        if "currency" in fields and fields["currency"] is not None and fields["currency"].strip().upper() != source["currency"].upper():
+            return fail("validation_error", "correction currency must match parent valuation")
+        if "valuation_kind" in fields and fields["valuation_kind"] is not None and fields["valuation_kind"].strip().lower() != source["valuation_kind"]:
+            return fail("validation_error", "correction valuation_kind must match parent valuation")
        preserved = {name: fields[name] if name in fields else source[name] for name in (
            "currency", "valuation_kind", "source_type", "source_ref", "verification_status", "memo", "as_of_at", "timezone",
            "cost_basis", "net_contributed", "reported_principal_kind", "reported_principal_label", "profit", "return_rate", "return_basis", "principal", "interest", "fee", "representative_liability_definition")}