mirror of
https://github.com/we-promise/sure.git
synced 2026-05-08 05:04:59 +00:00
* Fix SimpleFIN inverting Loan account balances
SimplefinAccount::Processor#process_account! routes every liability
through OverpaymentAnalyzer + normalize_liability_balance. That path
is built around credit-like liabilities, where transaction history
distinguishes debt vs. credit. For a Loan account with only the
opening anchor (no payment history), the analyzer returns :unknown
and the fallback negates the observed value:
def normalize_liability_balance(observed, bal, avail)
...
-observed
end
That's wrong for loans: the bank reports the principal outstanding
as a positive number from its own books. Negating it stores the loan
balance as negative, so BalanceSheet#net_worth = assets - liabilities
ends up _adding_ the loan instead of subtracting it (off by 2× the
loan amount). Example with a hypothetical mortgage:
raw_balance = 100000.00 (positive — bank's own report)
Sure stored = -100000.00 (negated by the fallback)
Net worth shown = inflated by 2 × 100000
Short-circuit Loan accountables straight to observed.abs and skip the
analyzer/fallback entirely. Loans don't have credit-vs-debt
ambiguity — if the loan is paid off the balance is 0, not negative.
Credit cards still go through the existing heuristic.
* Add observability for the SimpleFIN loan sign branch
Mirrors the logging + Sentry breadcrumb the credit-card branches emit
when the OverpaymentAnalyzer classifies as :credit / :debt, so the
loan short-circuit shows up in production traces too. Per CodeRabbit
review on #1574.
* Test that positive bank-reported loan balances are preserved
The existing "inverts negative balance for loan liabilities" test only
covers a bank that reports the loan as negative — both the old (buggy)
fallback and the new short-circuit produce the same +50000 there, which
is why the inversion bug went undetected. Add a sibling test where the
bank reports +50000 (the common mortgage convention); under the old
code that became -50000 and inflated net worth.
* Redact monetary amounts from SimpleFIN liability info logs
Move raw observed/stored amounts and metric totals from `Rails.logger.info`
and `Sentry.add_breadcrumb` payloads to a `Rails.logger.debug` line.
The info-level message and breadcrumb data now carry only identifiers
(`sfa_id`) plus the classification (`loan` / `credit` / `debt` /
`unknown`) and `tx_count`, so log aggregators and Sentry no longer
receive raw monetary values for any of the four liability branches.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
236 lines
9.5 KiB
Ruby
236 lines
9.5 KiB
Ruby
class SimplefinAccount::Processor
|
||
include SimplefinNumericHelpers
|
||
attr_reader :simplefin_account, :skipped_entries
|
||
|
||
def initialize(simplefin_account)
|
||
@simplefin_account = simplefin_account
|
||
@skipped_entries = []
|
||
end
|
||
|
||
# Each step represents different SimpleFin data processing
|
||
# Processing the account is the first step and if it fails, we halt
|
||
# Each subsequent step can fail independently, but we continue processing
|
||
def process
|
||
# If the account is missing (e.g., user deleted the connection and re‑linked later),
|
||
# do not auto‑link. Relinking is now a manual, user‑confirmed flow via the Relink modal.
|
||
unless simplefin_account.current_account.present?
|
||
return
|
||
end
|
||
|
||
process_account!
|
||
# Ensure provider link exists after processing the account/balance
|
||
begin
|
||
simplefin_account.ensure_account_provider!
|
||
rescue => e
|
||
Rails.logger.warn("SimpleFin provider link ensure failed for #{simplefin_account.id}: #{e.class} - #{e.message}")
|
||
end
|
||
process_transactions
|
||
process_investments
|
||
process_liabilities
|
||
end
|
||
|
||
private
|
||
|
||
def process_account!
|
||
# This should not happen in normal flow since accounts are created manually
|
||
# during setup, but keeping as safety check
|
||
if simplefin_account.current_account.blank?
|
||
Rails.logger.error("SimpleFin account #{simplefin_account.id} has no associated Account - this should not happen after manual setup")
|
||
return
|
||
end
|
||
|
||
# Update account balance and cash balance from latest SimpleFin data
|
||
account = simplefin_account.current_account
|
||
|
||
# Extract raw values from SimpleFIN snapshot
|
||
bal = to_decimal(simplefin_account.current_balance)
|
||
avail = to_decimal(simplefin_account.available_balance)
|
||
|
||
# Choose an observed value prioritizing posted balance first
|
||
# Use available_balance only when current_balance is truly missing (nil),
|
||
# not when it's explicitly zero (e.g., dormant credit card with no debt)
|
||
observed = simplefin_account.current_balance.nil? ? avail : bal
|
||
|
||
# Determine if this should be treated as a liability for normalization
|
||
is_linked_liability = [ "CreditCard", "Loan" ].include?(account.accountable_type)
|
||
raw = (simplefin_account.raw_payload || {}).with_indifferent_access
|
||
org = (simplefin_account.org_data || {}).with_indifferent_access
|
||
inferred = Simplefin::AccountTypeMapper.infer(
|
||
name: simplefin_account.name,
|
||
holdings: raw[:holdings],
|
||
extra: simplefin_account.extra,
|
||
balance: bal,
|
||
available_balance: avail,
|
||
institution: org[:name]
|
||
) rescue nil
|
||
is_mapper_liability = inferred && [ "CreditCard", "Loan" ].include?(inferred.accountable_type)
|
||
is_liability =
|
||
if account.accountable_type.present?
|
||
is_linked_liability
|
||
else
|
||
is_mapper_liability
|
||
end
|
||
|
||
if is_mapper_liability && !is_linked_liability
|
||
Rails.logger.warn(
|
||
"SimpleFIN account type mismatch: linked account #{account.id} type=#{account.accountable_type} " \
|
||
"differs from mapper inference (#{inferred.accountable_type}). Using linked account type."
|
||
)
|
||
end
|
||
|
||
balance = observed
|
||
if is_liability
|
||
# Loans report principal outstanding as a positive balance from the
|
||
# bank's perspective. The OverpaymentAnalyzer is designed for credit-
|
||
# like liabilities (where charges/payments distinguish debt vs.
|
||
# credit) and returns :unknown for loans with no transaction
|
||
# history, which then falls through to a fallback that negates the
|
||
# observed value — inverting the meaning so the loan ends up
|
||
# _adding_ to net worth instead of subtracting from it. Trust the
|
||
# bank's sign for Loans and skip the heuristic.
|
||
if account.accountable_type == "Loan"
|
||
balance = observed.abs
|
||
Rails.logger.info(
|
||
"SimpleFIN liability sign: classification=loan sfa=#{simplefin_account.id}"
|
||
)
|
||
Rails.logger.debug(
|
||
"SimpleFIN liability sign (loan) amounts: sfa=#{simplefin_account.id} " \
|
||
"observed=#{observed.to_s('F')} stored=#{balance.to_s('F')}"
|
||
)
|
||
Sentry.add_breadcrumb(Sentry::Breadcrumb.new(
|
||
category: "simplefin",
|
||
message: "liability_sign=loan",
|
||
data: { sfa_id: simplefin_account.id }
|
||
)) rescue nil
|
||
else
|
||
# 1) Try transaction-history heuristic when enabled
|
||
begin
|
||
result = SimplefinAccount::Liabilities::OverpaymentAnalyzer
|
||
.new(simplefin_account, observed_balance: observed)
|
||
.call
|
||
|
||
case result.classification
|
||
when :credit
|
||
balance = -observed.abs
|
||
Rails.logger.info(
|
||
"SimpleFIN overpayment heuristic: classified as credit for sfa=#{simplefin_account.id} " \
|
||
"tx_count=#{result.metrics[:tx_count]}"
|
||
)
|
||
Rails.logger.debug(
|
||
"SimpleFIN overpayment heuristic (credit) amounts: sfa=#{simplefin_account.id} " \
|
||
"observed=#{observed.to_s('F')} metrics=#{result.metrics.slice(:charges_total, :payments_total, :tx_count).inspect}"
|
||
)
|
||
Sentry.add_breadcrumb(Sentry::Breadcrumb.new(
|
||
category: "simplefin",
|
||
message: "liability_sign=credit",
|
||
data: { sfa_id: simplefin_account.id }
|
||
)) rescue nil
|
||
when :debt
|
||
balance = observed.abs
|
||
Rails.logger.info(
|
||
"SimpleFIN overpayment heuristic: classified as debt for sfa=#{simplefin_account.id} " \
|
||
"tx_count=#{result.metrics[:tx_count]}"
|
||
)
|
||
Rails.logger.debug(
|
||
"SimpleFIN overpayment heuristic (debt) amounts: sfa=#{simplefin_account.id} " \
|
||
"observed=#{observed.to_s('F')} metrics=#{result.metrics.slice(:charges_total, :payments_total, :tx_count).inspect}"
|
||
)
|
||
Sentry.add_breadcrumb(Sentry::Breadcrumb.new(
|
||
category: "simplefin",
|
||
message: "liability_sign=debt",
|
||
data: { sfa_id: simplefin_account.id }
|
||
)) rescue nil
|
||
else
|
||
# 2) Fall back to existing sign-only logic (log unknown for observability)
|
||
begin
|
||
Rails.logger.info(
|
||
"SimpleFIN overpayment heuristic: unknown for sfa=#{simplefin_account.id} " \
|
||
"reason=#{result.reason} tx_count=#{result.metrics[:tx_count]}; falling back"
|
||
)
|
||
Rails.logger.debug(
|
||
"SimpleFIN overpayment heuristic (unknown) amounts: sfa=#{simplefin_account.id} " \
|
||
"observed=#{observed.to_s('F')} " \
|
||
"charges_total=#{result.metrics[:charges_total]} payments_total=#{result.metrics[:payments_total]}"
|
||
)
|
||
rescue
|
||
# no-op
|
||
end
|
||
balance = normalize_liability_balance(observed, bal, avail)
|
||
end
|
||
rescue NameError
|
||
# Analyzer not loaded; keep legacy behavior
|
||
balance = normalize_liability_balance(observed, bal, avail)
|
||
rescue => e
|
||
Rails.logger.warn("SimpleFIN overpayment heuristic error for sfa=#{simplefin_account.id}: #{e.class} - #{e.message}")
|
||
balance = normalize_liability_balance(observed, bal, avail)
|
||
end
|
||
end
|
||
end
|
||
|
||
# Calculate cash balance correctly for investment accounts
|
||
cash_balance = if account.accountable_type == "Investment"
|
||
calculator = SimplefinAccount::Investments::BalanceCalculator.new(simplefin_account)
|
||
calculator.cash_balance
|
||
else
|
||
balance
|
||
end
|
||
|
||
account.update!(
|
||
balance: balance,
|
||
cash_balance: cash_balance,
|
||
currency: simplefin_account.currency
|
||
)
|
||
end
|
||
|
||
def process_transactions
|
||
processor = SimplefinAccount::Transactions::Processor.new(simplefin_account)
|
||
processor.process
|
||
@skipped_entries.concat(processor.skipped_entries)
|
||
rescue => e
|
||
report_exception(e, "transactions")
|
||
end
|
||
|
||
def process_investments
|
||
return unless simplefin_account.current_account&.accountable_type == "Investment"
|
||
SimplefinAccount::Investments::TransactionsProcessor.new(simplefin_account).process
|
||
SimplefinAccount::Investments::HoldingsProcessor.new(simplefin_account).process
|
||
rescue => e
|
||
report_exception(e, "investments")
|
||
end
|
||
|
||
def process_liabilities
|
||
case simplefin_account.current_account&.accountable_type
|
||
when "CreditCard"
|
||
SimplefinAccount::Liabilities::CreditProcessor.new(simplefin_account).process
|
||
when "Loan"
|
||
SimplefinAccount::Liabilities::LoanProcessor.new(simplefin_account).process
|
||
end
|
||
rescue => e
|
||
report_exception(e, "liabilities")
|
||
end
|
||
|
||
def report_exception(error, context)
|
||
Sentry.capture_exception(error) do |scope|
|
||
scope.set_tags(
|
||
simplefin_account_id: simplefin_account.id,
|
||
context: context
|
||
)
|
||
end
|
||
end
|
||
|
||
# Helpers
|
||
# to_decimal and same_sign? provided by SimplefinNumericHelpers concern
|
||
|
||
def normalize_liability_balance(observed, bal, avail)
|
||
both_present = bal.nonzero? && avail.nonzero?
|
||
if both_present && same_sign?(bal, avail)
|
||
if bal.positive? && avail.positive?
|
||
return -observed.abs
|
||
elsif bal.negative? && avail.negative?
|
||
return observed.abs
|
||
end
|
||
end
|
||
-observed
|
||
end
|
||
end
|