Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
232 changes: 232 additions & 0 deletions app/models/simplefin_account/liabilities/overpayment_analyzer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,232 @@
# frozen_string_literal: true

# Classifies a SimpleFIN liability balance as :debt (owe, show positive)
# or :credit (overpaid, show negative) using recent transaction history.
#
# Notes:
# - Preferred signal: already-imported Entry records for the linked Account
# (they are in Maybe's convention: expenses/charges > 0, payments < 0).
# - Fallback signal: provider raw transactions payload with amounts converted
# to Maybe convention by negating SimpleFIN's banking convention.
# - Returns :unknown when evidence is insufficient; callers should fallback
# to existing sign-only normalization.
class SimplefinAccount::Liabilities::OverpaymentAnalyzer
Result = Struct.new(:classification, :reason, :metrics, keyword_init: true)

DEFAULTS = {
window_days: 120,
min_txns: 10,
min_payments: 2,
epsilon_base: BigDecimal("0.50"),
statement_guard_days: 5,
sticky_days: 7
}.freeze

def initialize(simplefin_account, observed_balance:, now: Time.current)
@sfa = simplefin_account
@observed = to_d(observed_balance)
@now = now
end

def call
return unknown("flag disabled") unless enabled?
return unknown("no-account") unless (account = @sfa.current_account)

# Only applicable for liabilities
return unknown("not-liability") unless %w[CreditCard Loan].include?(account.accountable_type)

# Near-zero observed balances are too noisy to infer
return unknown("near-zero-balance") if @observed.abs <= epsilon_base

# Sticky cache via Rails.cache to avoid DB schema changes
sticky = read_sticky
if sticky && sticky[:expires_at] > @now
return Result.new(classification: sticky[:value].to_sym, reason: "sticky_hint", metrics: {})
end

txns = gather_transactions(account)
return unknown("insufficient-txns") if txns.size < min_txns

metrics = compute_metrics(txns)
cls, reason = classify(metrics)

if %i[credit debt].include?(cls)
write_sticky(cls)
end

Result.new(classification: cls, reason: reason, metrics: metrics)
end

private

def enabled?
env = ENV["SIMPLEFIN_CC_OVERPAYMENT_HEURISTIC"].to_s
env_specified = env.present?
env_enabled = env_specified ? (%w[1 true yes on].include?(env.downcase)) : false
# Allow dynamic Setting override; Setting[] returns value or nil
setting_val = Setting["simplefin_cc_overpayment_detection"]
setting_enabled = case setting_val
when true then true
when false then false
when String then %w[1 true yes on].include?(setting_val.downcase)
else
nil
end
# Default behavior: ENABLED unless explicitly disabled via Setting or ENV
if setting_enabled.nil?
env_specified ? env_enabled : true
else
setting_enabled
end
end
Comment on lines 63 to 80
Copy link

@coderabbitai coderabbitai bot Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check the file context and method to understand intent
cat -n app/models/simplefin_account/liabilities/overpayment_analyzer.rb | head -100

Repository: we-promise/sure

Length of output: 4105


🏁 Script executed:

# Search for any documentation or comments about the feature rollout
rg -i "overpayment" --type rb -B2 -A2 | head -50

Repository: we-promise/sure

Length of output: 85


🏁 Script executed:

# Look for other feature flag patterns in the codebase
rg "ENV\[" --type rb | grep -i "feature\|flag\|heuristic" | head -20

Repository: we-promise/sure

Length of output: 85


🏁 Script executed:

# Check for related settings or configuration
rg "simplefin_cc_overpayment" --type rb -B2 -A2

Repository: we-promise/sure

Length of output: 85


🏁 Script executed:

# Look for any PR or issue documentation
find . -type f \( -name "*.md" -o -name "*.txt" \) -exec rg -l "overpayment" {} \;

Repository: we-promise/sure

Length of output: 85


🏁 Script executed:

# Search for overpayment references without file type restrictions
grep -r "overpayment" . --include="*.md" --include="*.rb" --include="*.txt" 2>/dev/null | head -30

Repository: we-promise/sure

Length of output: 3552


🏁 Script executed:

# Check the documentation file that was found
cat docs/onboarding/guide.md | grep -i "overpayment\|simplefin" -B2 -A2 | head -40

Repository: we-promise/sure

Length of output: 572


🏁 Script executed:

# Search for Setting keys related to overpayment to understand operational control
grep -r "simplefin_cc_overpayment" . 2>/dev/null

Repository: we-promise/sure

Length of output: 2022


🏁 Script executed:

# Look for any git history or comments about feature rollout
grep -r "feature.*flag\|gradual.*rollout\|beta\|experimental" . --include="*.rb" --include="*.md" 2>/dev/null | grep -i "overpayment\|heuristic" | head -20

Repository: we-promise/sure

Length of output: 163


🏁 Script executed:

# Check if there are any specs or tests for the enabled? method
grep -r "enabled\?" . --include="*spec.rb" --include="*test.rb" 2>/dev/null | grep -i "overpayment" -B2 -A2

Repository: we-promise/sure

Length of output: 41


🏁 Script executed:

cat test/models/simplefin_account/liabilities/overpayment_analyzer_test.rb

Repository: we-promise/sure

Length of output: 3919


🏁 Script executed:

# Check the importer.rb context around the "feature-flagged" comment
grep -B5 -A10 "feature-flagged" ./app/models/simplefin_item/importer.rb

Repository: we-promise/sure

Length of output: 676


Default behavior should be DISABLED for a feature-flagged heuristic.

The test setup explicitly enables this with Setting["simplefin_cc_overpayment_detection"] = "true" and disables it in teardown to prevent test bleeding. This pattern confirms the feature should default to off, not on. The importer.rb comment also labels it "feature-flagged," which typically implies opt-in behavior. Change the default at line 77 from true to false:

-      if setting_enabled.nil?
-        env_specified ? env_enabled : true
+      if setting_enabled.nil?
+        env_specified ? env_enabled : false
🤖 Prompt for AI Agents
In app/models/simplefin_account/liabilities/overpayment_analyzer.rb around lines
62–81, the method enabled? currently defaults to true when no ENV or Setting is
specified; change the default to disabled by replacing the final branch so that
when setting_enabled.nil? it returns env_specified ? env_enabled : false (i.e.
default false), ensuring feature-flagged heuristic is opt-in and matches tests
that explicitly enable/disable it.

Copy link
Collaborator Author

@luckyPipewrench luckyPipewrench Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping default ON is intentional. If SimpleFin fixes how they send the balances, we can later change the default globally, it’s a one‑line change.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luckyPipewrench, understood! That makes sense — keeping it enabled by default allows you to quickly disable the normalization globally if SimpleFin changes their sign semantics upstream. The single-toggle approach is pragmatic for this scenario.


🧠 Learnings used
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.

Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.

Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.669Z
Learning: Account sync must perform transfer auto-matching, calculate daily balance records from start_date to Date.current, calculate holdings, and optionally enrich transaction data

Learnt from: Himmelschmidt
Repo: we-promise/sure PR: 104
File: app/models/simplefin_item/importer.rb:34-45
Timestamp: 2025-09-03T01:15:59.638Z
Learning: In SimpleFin integration, the default initial sync period is intentionally set to 7 days (not 3 years) to provide quick setup, with users able to optionally extend to 3 years via sync_start_date selection. This design balances setup speed with comprehensive data access options.

Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.


def window_days
val = Setting["simplefin_cc_overpayment_window_days"]
v = (val.presence || DEFAULTS[:window_days]).to_i
v > 0 ? v : DEFAULTS[:window_days]
end

def min_txns
val = Setting["simplefin_cc_overpayment_min_txns"]
v = (val.presence || DEFAULTS[:min_txns]).to_i
v > 0 ? v : DEFAULTS[:min_txns]
end

def min_payments
val = Setting["simplefin_cc_overpayment_min_payments"]
v = (val.presence || DEFAULTS[:min_payments]).to_i
v > 0 ? v : DEFAULTS[:min_payments]
end

def epsilon_base
val = Setting["simplefin_cc_overpayment_epsilon_base"]
d = to_d(val.presence || DEFAULTS[:epsilon_base])
d > 0 ? d : DEFAULTS[:epsilon_base]
end

def statement_guard_days
val = Setting["simplefin_cc_overpayment_statement_guard_days"]
v = (val.presence || DEFAULTS[:statement_guard_days]).to_i
v >= 0 ? v : DEFAULTS[:statement_guard_days]
end

def sticky_days
val = Setting["simplefin_cc_overpayment_sticky_days"]
v = (val.presence || DEFAULTS[:sticky_days]).to_i
v > 0 ? v : DEFAULTS[:sticky_days]
end

def gather_transactions(account)
start_date = (@now.to_date - window_days.days)

# Prefer materialized entries
entries = account.entries.where("date >= ?", start_date).select(:amount, :date)
txns = entries.map { |e| { amount: to_d(e.amount), date: e.date } }
return txns if txns.size >= min_txns

# Fallback: provider raw payload
raw = Array(@sfa.raw_transactions_payload)
raw_txns = raw.filter_map do |tx|
h = tx.with_indifferent_access
amt = convert_provider_amount(h[:amount])
d = (
Simplefin::DateUtils.parse_provider_date(h[:posted]) ||
Simplefin::DateUtils.parse_provider_date(h[:transacted_at])
)
next nil unless d
next nil if d < start_date
{ amount: amt, date: d }
end
raw_txns
rescue => _e
[]
end

def compute_metrics(txns)
charges = BigDecimal("0")
payments = BigDecimal("0")
payments_count = 0
recent_payment = false
guard_since = (@now.to_date - statement_guard_days.days)

txns.each do |t|
amt = to_d(t[:amount])
if amt.positive?
charges += amt
elsif amt.negative?
payments += -amt
payments_count += 1
recent_payment ||= (t[:date] >= guard_since)
end
end

net = charges - payments
{
charges_total: charges,
payments_total: payments,
payments_count: payments_count,
tx_count: txns.size,
net: net,
observed: @observed,
window_days: window_days,
recent_payment: recent_payment
}
end

def classify(m)
# Boundary guard: a single very recent payment may create temporary credit before charges post
if m[:recent_payment] && m[:payments_count] <= 2
return [ :unknown, "statement-guard" ]
end

eps = [ epsilon_base, (@observed.abs * BigDecimal("0.005")) ].max

# Overpayment (credit): payments exceed charges by at least the observed balance (within eps)
if (m[:payments_total] - m[:charges_total]) >= (@observed.abs - eps)
return [ :credit, "payments>=charges+observed-eps" ]
end

# Debt: charges exceed payments beyond epsilon
if (m[:charges_total] - m[:payments_total]) > eps && m[:payments_count] >= min_payments
return [ :debt, "charges>payments+eps" ]
end

[ :unknown, "ambiguous" ]
end

def convert_provider_amount(val)
amt = case val
when String then BigDecimal(val) rescue BigDecimal("0")
when Numeric then BigDecimal(val.to_s)
else BigDecimal("0")
end
# Negate to convert banking convention (expenses negative) -> Maybe convention
-amt
end

def read_sticky
Rails.cache.read(sticky_key)
end

def write_sticky(value)
Rails.cache.write(sticky_key, { value: value.to_s, expires_at: @now + sticky_days.days }, expires_in: sticky_days.days)
end

def sticky_key
"simplefin:sfa:#{@sfa.id}:liability_sign_hint"
end

def to_d(value)
case value
when BigDecimal then value
when String then BigDecimal(value) rescue BigDecimal("0")
when Numeric then BigDecimal(value.to_s)
else
BigDecimal("0")
end
end

def unknown(reason)
Result.new(classification: :unknown, reason: reason, metrics: {})
end
end
119 changes: 110 additions & 9 deletions app/models/simplefin_account/processor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,88 @@ def process_account!

# Update account balance and cash balance from latest SimpleFin data
account = simplefin_account.current_account
balance = simplefin_account.current_balance || simplefin_account.available_balance || 0

# Normalize balances for liabilities (SimpleFIN typically uses opposite sign)
# App convention:
# - Liabilities: positive => you owe; negative => provider owes you (overpayment/credit)
# Since providers often send the opposite sign, ALWAYS invert for liabilities so
# that both debt and overpayment cases are represented correctly.
if [ "CreditCard", "Loan" ].include?(account.accountable_type)
balance = -balance

# 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
observed = bal.nonzero? ? bal : avail

# Determine if this should be treated as a liability for normalization
is_linked_liability = [ "CreditCard", "Loan" ].include?(account.accountable_type)
inferred = Simplefin::AccountTypeMapper.infer(
name: simplefin_account.name,
holdings: (simplefin_account.raw_payload || {})["holdings"],
extra: simplefin_account.extra,
balance: bal,
available_balance: avail,
institution: simplefin_account.org_data&.dig("name")
) rescue nil
is_mapper_liability = inferred && [ "CreditCard", "Loan" ].include?(inferred.accountable_type)
is_liability = is_linked_liability || is_mapper_liability

if is_mapper_liability && !is_linked_liability
Rails.logger.warn(
"SimpleFIN liability normalization: linked account #{account.id} type=#{account.accountable_type} " \
"appears to be liability via mapper (#{inferred.accountable_type}). Normalizing as liability; consider relinking."
)
end

balance = observed
if is_liability
# 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
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

# Calculate cash balance correctly for investment accounts
Expand Down Expand Up @@ -98,4 +171,32 @@ def report_exception(error, context)
)
end
end

# Helpers
def to_decimal(value)
return BigDecimal("0") if value.nil?
case value
when BigDecimal then value
when String then BigDecimal(value) rescue BigDecimal("0")
when Numeric then BigDecimal(value.to_s)
else
BigDecimal("0")
end
end

def same_sign?(a, b)
(a.positive? && b.positive?) || (a.negative? && b.negative?)
end

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
Loading