From 7c14c80444c34e16221ab60c7b49cf3735ef788b Mon Sep 17 00:00:00 2001 From: GermanDZ Date: Wed, 29 Apr 2026 13:00:38 +0200 Subject: [PATCH] Fix SimpleFIN inverting Loan account balances (#1574) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- app/models/simplefin_account/processor.rb | 127 +++++++++++------- .../simplefin_account_processor_test.rb | 26 ++++ 2 files changed, 106 insertions(+), 47 deletions(-) diff --git a/app/models/simplefin_account/processor.rb b/app/models/simplefin_account/processor.rb index f9ef87641..98cba6497 100644 --- a/app/models/simplefin_account/processor.rb +++ b/app/models/simplefin_account/processor.rb @@ -80,57 +80,90 @@ class SimplefinAccount::Processor balance = observed if is_liability - # 1) Try transaction-history heuristic when enabled - begin - result = SimplefinAccount::Liabilities::OverpaymentAnalyzer - .new(simplefin_account, observed_balance: observed) - .call + # 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}, " \ - "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, observed: observed.to_s("F") } - )) rescue nil - when :debt - balance = observed.abs - Rails.logger.info( - "SimpleFIN overpayment heuristic: classified as debt for 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, observed: observed.to_s("F") } - )) rescue nil - else - # 2) Fall back to existing sign-only logic (log unknown for observability) - begin - obs = { - reason: result.reason, - tx_count: result.metrics[:tx_count], - charges_total: result.metrics[:charges_total], - payments_total: result.metrics[:payments_total], - observed: observed.to_s("F") - }.compact - Rails.logger.info("SimpleFIN overpayment heuristic: unknown; falling back #{obs.inspect}") - rescue - # no-op + 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 - 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 diff --git a/test/models/simplefin_account_processor_test.rb b/test/models/simplefin_account_processor_test.rb index 0c6a33b88..b5361b365 100644 --- a/test/models/simplefin_account_processor_test.rb +++ b/test/models/simplefin_account_processor_test.rb @@ -64,6 +64,32 @@ class SimplefinAccountProcessorTest < ActiveSupport::TestCase assert_equal BigDecimal("50000"), acct.reload.balance end + # Many banks (and most mortgage feeds) report the loan principal + # outstanding as a positive number from the bank's books. The old + # liability path ran every Loan through OverpaymentAnalyzer + + # normalize_liability_balance, and the latter's fallback for + # `:unknown` classifications returned `-observed`, flipping a + # bank-reported `+50000` into `-50000`. That bug made loans _add_ to + # net worth instead of subtracting, so this test pins the corrected + # behaviour. + test "preserves positive provider balance for loan liabilities" do + sfin_acct = SimplefinAccount.create!( + simplefin_item: @item, + name: "Mortgage", + account_id: "loan_2", + currency: "USD", + account_type: "mortgage", + current_balance: BigDecimal("50000") + ) + + acct = accounts(:loan) + acct.update!(simplefin_account: sfin_acct) + + SimplefinAccount::Processor.new(sfin_acct).send(:process_account!) + + assert_equal BigDecimal("50000"), acct.reload.balance + end + test "positive provider balance (overpayment) becomes negative for credit card liabilities" do sfin_acct = SimplefinAccount.create!( simplefin_item: @item,