diff --git a/.claude/rules/providers.md b/.claude/rules/providers.md index d7cb1845..bb44b7b4 100644 --- a/.claude/rules/providers.md +++ b/.claude/rules/providers.md @@ -53,6 +53,7 @@ Connection storage uses generic columns: `external_id` + `encrypted_credentials` - **`external_id` is minted** (`internal/shortid`) — SimpleFIN exposes no stable upstream id and a single access URL spans many banks. Reauth keeps the row and rotates only `encrypted_credentials` (`UpdateBankConnectionCredentials`). - **Sign is inverted vs Plaid**: SimpleFIN positive = money in; negate uniformly (no per-account-type rule, unlike Teller). - **Sync**: date-range polling chunked into **≤90-day windows** (bridge cap). Cursor = last-sync RFC3339 timestamp. `ReconcilesPendingByPolling()` is true. New connections default to a **daily** `sync_interval_override_minutes` (bridge expects ≤24 req/day). +- **Account discovery on sync.** One access URL spans every bank at the bridge, and that set grows as the user links more banks there. So each sync returns the connection's full current account set in `SyncResult.Accounts`; the engine upserts it (metadata only, via `UpsertAccountMetadata` — never balances) before processing transactions, so banks added at the bridge after connect appear automatically on the next sync — no reconnect needed. Fixed-account-set providers (Plaid, CSV) leave `SyncResult.Accounts` nil and the engine skips the upsert. - `HandleWebhook`/`CreateReauthSession` → `ErrNotSupported`; `RemoveConnection` → `nil` (no revoke endpoint). Account type defaults to `depository` (not exposed by SimpleFIN). - **Opt-in**: gated by `SIMPLEFIN_ENABLED` env / `simplefin_enabled` app_config (no server-level credential). Admin-page connect/relink only in v1 — no REST endpoint, not in the hosted-link allowlist. diff --git a/docs/simplefin-integration.md b/docs/simplefin-integration.md index 7199d1a1..828b6fac 100644 --- a/docs/simplefin-integration.md +++ b/docs/simplefin-integration.md @@ -58,6 +58,20 @@ window each sync, the engine soft-deletes stale pending rows via the `ReconcilesPendingByPolling()` capability (shared with Teller — see `.claude/rules/providers.md`). +### Account discovery on every sync + +One access URL spans every bank at the bridge, and the user can link more banks +there *after* connecting Breadbox. So `SyncTransactions` also returns the +connection's **full current account set** in `SyncResult.Accounts` (captured once +per sync — the bridge lists the full set on every window, so there's no need to +re-collect per window). Before processing transactions, the sync engine upserts +that set with `UpsertAccountMetadata` — **metadata only, never balances** (those +are owned by the balance-refresh path) and `connection_id` is set only on INSERT, +so an existing account keeps its connection. A bank added at the bridge after +connect therefore appears in Breadbox on the next sync automatically — no +reconnect, no new token. Providers whose account set is fixed at connect time +(Plaid, CSV) leave `SyncResult.Accounts` nil and the engine skips the upsert. + ### Rate limits The SimpleFIN Bridge expects **≤ 24 requests/day** ("daily updates"). New diff --git a/internal/db/queries/accounts.sql b/internal/db/queries/accounts.sql index 7b732107..4f9a9452 100644 --- a/internal/db/queries/accounts.sql +++ b/internal/db/queries/accounts.sql @@ -13,6 +13,26 @@ ON CONFLICT (external_account_id) DO UPDATE SET updated_at = NOW() RETURNING *; +-- name: UpsertAccountMetadata :one +-- Inserts a freshly discovered account or refreshes the metadata of an existing +-- one WITHOUT touching balances. Used by the sync engine when a provider returns +-- its current account set mid-sync (e.g. SimpleFIN, where one access URL grows +-- new accounts as the user links banks at the bridge). Balances are owned by the +-- separate balance-refresh path, so this upsert deliberately omits them to avoid +-- clobbering live values with NULLs. connection_id is set only on INSERT; an +-- existing account keeps its connection. +INSERT INTO accounts (connection_id, external_account_id, name, official_name, type, subtype, mask, iso_currency_code) +VALUES ($1, $2, $3, $4, $5, $6, $7, $8) +ON CONFLICT (external_account_id) DO UPDATE SET + name = EXCLUDED.name, + official_name = EXCLUDED.official_name, + type = EXCLUDED.type, + subtype = EXCLUDED.subtype, + mask = EXCLUDED.mask, + iso_currency_code = EXCLUDED.iso_currency_code, + updated_at = NOW() +RETURNING id, external_account_id, name, display_name; + -- name: ListAccountsByConnection :many SELECT * FROM accounts WHERE connection_id = $1 ORDER BY name; diff --git a/internal/provider/provider.go b/internal/provider/provider.go index 94e11d66..d4e75084 100644 --- a/internal/provider/provider.go +++ b/internal/provider/provider.go @@ -83,6 +83,16 @@ type SyncResult struct { Removed []string // external transaction IDs Cursor string HasMore bool + + // Accounts is the provider's current account set, returned by providers + // whose account list can change between connect and sync (notably SimpleFIN, + // where one access URL spans every bank the user links at their bridge and + // grows new accounts over time). The sync engine upserts these onto the + // connection — metadata only, no balances — before processing transactions, + // so transactions for newly-appeared accounts resolve instead of being + // dropped. Leave nil for providers whose account set is fixed at connect + // time (Plaid, CSV); the engine simply skips the upsert when empty. + Accounts []Account } type Transaction struct { diff --git a/internal/provider/simplefin/simplefin_test.go b/internal/provider/simplefin/simplefin_test.go index c749a7b3..1f31e3e5 100644 --- a/internal/provider/simplefin/simplefin_test.go +++ b/internal/provider/simplefin/simplefin_test.go @@ -213,6 +213,41 @@ func TestSyncTransactions_MappingAndSigns(t *testing.T) { } } +func TestSyncTransactions_ReturnsDiscoveredAccounts(t *testing.T) { + // Two banks under one access URL — the SimpleFIN aggregator case. The sync + // must surface the full account set so the engine can pick up banks the user + // linked at the bridge after the initial connect. + fb := newFakeBridge(t, `{"errors":[],"accounts":[ + {"org":{"name":"Bank A"},"id":"acct-1","name":"Checking","currency":"USD","balance":"50.00","transactions":[]}, + {"org":{"name":"Bank B"},"id":"acct-2","name":"Savings","currency":"USD","balance":"100.00","transactions":[]} + ]}`) + p := testProvider() + enc, _ := crypto.Encrypt([]byte(fb.accessURL()), testKey) + conn := provider.Connection{ProviderName: "simplefin", EncryptedCredentials: enc} + + // Empty cursor → multi-window backfill. Every window re-returns the full + // account list, so SyncTransactions must take the union deduped by external + // id — each account appears exactly once in the result, not one copy per + // window. + res, err := p.SyncTransactions(context.Background(), conn, "") + if err != nil { + t.Fatalf("SyncTransactions: %v", err) + } + if fb.accountsHits < 2 { + t.Fatalf("expected multiple windows for a backfill, got %d hits", fb.accountsHits) + } + if len(res.Accounts) != 2 { + t.Fatalf("accounts = %d, want 2 (deduped across %d windows)", len(res.Accounts), fb.accountsHits) + } + byID := map[string]provider.Account{} + for _, a := range res.Accounts { + byID[a.ExternalID] = a + } + if byID["acct-1"].Name != "Checking" || byID["acct-2"].Name != "Savings" { + t.Errorf("account names not mapped: %+v", res.Accounts) + } +} + func TestSyncTransactions_WindowsLongBackfill(t *testing.T) { fb := newFakeBridge(t, `{"errors":[],"accounts":[{"org":{"name":"Bank"},"id":"a","name":"Checking","currency":"USD","balance":"0","transactions":[]}]}`) p := testProvider() diff --git a/internal/provider/simplefin/sync.go b/internal/provider/simplefin/sync.go index 8ce4e504..216a5044 100644 --- a/internal/provider/simplefin/sync.go +++ b/internal/provider/simplefin/sync.go @@ -43,18 +43,38 @@ func (p *SimpleFINProvider) SyncTransactions(ctx context.Context, conn provider. } var allTxns []provider.Transaction + // Accumulate the account set as the union across windows, deduped by external + // id. The bridge lists the full account set on each window (the date range + // only bounds the nested transactions), so the union is the connection's + // complete current set — and taking the union rather than trusting a single + // window stays correct even if a window's view is ever partial. The engine + // upserts these (metadata only) so banks the user links at the bridge after + // connect are discovered on the next sync. + seenAccounts := make(map[string]struct{}) + var accounts []provider.Account for _, w := range windows(fromDate, now, maxWindowDays) { - txns, err := p.fetchWindow(ctx, accessURL, w.start, w.end) + txns, accts, err := p.fetchWindow(ctx, accessURL, w.start, w.end) if err != nil { return provider.SyncResult{}, err } allTxns = append(allTxns, txns...) + for _, a := range accts { + if a.ExternalID == "" { + continue + } + if _, ok := seenAccounts[a.ExternalID]; ok { + continue + } + seenAccounts[a.ExternalID] = struct{}{} + accounts = append(accounts, a) + } } return provider.SyncResult{ - Added: allTxns, - HasMore: false, - Cursor: now.Format(time.RFC3339), + Added: allTxns, + Accounts: accounts, + HasMore: false, + Cursor: now.Format(time.RFC3339), }, nil } @@ -71,8 +91,9 @@ func syncStart(cursor string, now time.Time) (time.Time, error) { } // fetchWindow fetches one date-bounded /accounts page and maps every nested -// transaction. start-date is inclusive, end-date is exclusive. -func (p *SimpleFINProvider) fetchWindow(ctx context.Context, accessURL string, start, end time.Time) ([]provider.Transaction, error) { +// transaction plus the account set it carries. start-date is inclusive, +// end-date is exclusive. +func (p *SimpleFINProvider) fetchWindow(ctx context.Context, accessURL string, start, end time.Time) ([]provider.Transaction, []provider.Account, error) { query := strings.Join([]string{ "start-date=" + strconv.FormatInt(start.Unix(), 10), "end-date=" + strconv.FormatInt(end.Unix(), 10), @@ -81,11 +102,13 @@ func (p *SimpleFINProvider) fetchWindow(ctx context.Context, accessURL string, s set, err := p.fetchAccountSet(ctx, accessURL, query) if err != nil { - return nil, err + return nil, nil, err } var txns []provider.Transaction + accounts := make([]provider.Account, 0, len(set.Accounts)) for _, acct := range set.Accounts { + accounts = append(accounts, acct.toAccount()) for _, t := range acct.Transactions { mapped, err := t.toTransaction(acct.ID, acct.Currency) if err != nil { @@ -96,7 +119,7 @@ func (p *SimpleFINProvider) fetchWindow(ctx context.Context, accessURL string, s txns = append(txns, mapped) } } - return txns, nil + return txns, accounts, nil } type window struct { diff --git a/internal/sync/engine.go b/internal/sync/engine.go index 3be7c149..665e73ea 100644 --- a/internal/sync/engine.go +++ b/internal/sync/engine.go @@ -269,6 +269,10 @@ func (e *Engine) runSync(ctx context.Context, connectionID pgtype.UUID, syncLogS var pendingRemovals []string var pendingAdded []provider.Transaction var pendingModified []provider.Transaction + // Accounts the provider re-discovered this sync (SimpleFIN spans a growing + // set of banks under one access URL). Deduped by external id across pages. + var pendingAccounts []provider.Account + seenAccountExternalIDs := make(map[string]struct{}) // Pagination loop. for { @@ -280,6 +284,8 @@ func (e *Engine) runSync(ctx context.Context, connectionID pgtype.UUID, syncLogS pendingRemovals = nil pendingAdded = nil pendingModified = nil + pendingAccounts = nil + seenAccountExternalIDs = make(map[string]struct{}) continue } if errors.Is(syncErr, provider.ErrReauthRequired) { @@ -299,6 +305,16 @@ func (e *Engine) runSync(ctx context.Context, connectionID pgtype.UUID, syncLogS pendingRemovals = append(pendingRemovals, result.Removed...) pendingAdded = append(pendingAdded, result.Added...) pendingModified = append(pendingModified, result.Modified...) + for _, acct := range result.Accounts { + if acct.ExternalID == "" { + continue + } + if _, ok := seenAccountExternalIDs[acct.ExternalID]; ok { + continue + } + seenAccountExternalIDs[acct.ExternalID] = struct{}{} + pendingAccounts = append(pendingAccounts, acct) + } if result.HasMore { cursor = result.Cursor @@ -323,6 +339,40 @@ func (e *Engine) runSync(ctx context.Context, connectionID pgtype.UUID, syncLogS txQueries := e.db.WithTx(tx) + // Discover accounts FIRST. Providers like SimpleFIN return their full + // account set on every sync; new accounts appear when the user links + // another bank at their bridge. Upsert metadata (never balances — those + // are owned by the balance-refresh path below) and seed the caches so + // the transactions for a brand-new account resolve in this same sync + // rather than erroring on a missing account row. + for _, acct := range pendingAccounts { + if _, known := accountIDCache[acct.ExternalID]; known { + continue // already linked to this connection; metadata refresh isn't worth a write per sync + } + row, err := txQueries.UpsertAccountMetadata(ctx, db.UpsertAccountMetadataParams{ + ConnectionID: connectionID, + ExternalAccountID: acct.ExternalID, + Name: acct.Name, + OfficialName: pgconv.TextIfNotEmpty(acct.OfficialName), + Type: acct.Type, + Subtype: pgconv.TextIfNotEmpty(acct.Subtype), + Mask: pgconv.TextIfNotEmpty(acct.Mask), + IsoCurrencyCode: pgconv.TextIfNotEmpty(acct.ISOCurrencyCode), + }) + if err != nil { + logger.Error("upsert discovered account", "external_id", acct.ExternalID, "error", err) + continue + } + accountIDCache[row.ExternalAccountID] = row.ID + key := pgconv.FormatUUID(row.ID) + if row.DisplayName.Valid && row.DisplayName.String != "" { + accountNameCache[key] = row.DisplayName.String + } else { + accountNameCache[key] = row.Name + } + logger.Info("discovered new account during sync", "external_id", row.ExternalAccountID, "name", row.Name) + } + // Process removed FIRST. for _, externalID := range pendingRemovals { if err := txQueries.SoftDeleteTransactionByExternalID(ctx, externalID); err != nil { diff --git a/internal/sync/engine_integration_test.go b/internal/sync/engine_integration_test.go index b081edff..745a2f2f 100644 --- a/internal/sync/engine_integration_test.go +++ b/internal/sync/engine_integration_test.go @@ -262,6 +262,89 @@ func TestSync_BasicAddTransactions(t *testing.T) { } } +// TestSync_DiscoversNewAccountsMidSync proves the SimpleFIN aggregator case: +// a connection that re-returns its account set on sync, where a NEW account has +// appeared (the user linked another bank at their bridge after connecting). The +// engine must create that account and link it to the connection so its +// transactions resolve instead of being dropped on a missing-account lookup. +func TestSync_DiscoversNewAccountsMidSync(t *testing.T) { + pool, queries := testutil.ServicePool(t) + ctx := context.Background() + + seedCategories(t, queries) + + user := testutil.MustCreateUser(t, queries, "Alice") + conn := testutil.MustCreateConnection(t, queries, user.ID, "item_simplefin") + // Only one account exists at connect time. + testutil.MustCreateAccount(t, queries, conn.ID, "ext_acct_1", "Checking") + + mock := &mockProvider{ + reconcilesPending: true, // poll-based, like SimpleFIN + syncResult: provider.SyncResult{ + Accounts: []provider.Account{ + {ExternalID: "ext_acct_1", Name: "Checking", Type: "depository", ISOCurrencyCode: "USD"}, + {ExternalID: "ext_acct_2", Name: "New Savings", Type: "depository", ISOCurrencyCode: "USD"}, + }, + Added: []provider.Transaction{ + { + ExternalID: "txn_on_new", + AccountExternalID: "ext_acct_2", // belongs to the just-discovered account + Amount: decimal.NewFromFloat(20.00), + Date: time.Date(2025, 3, 1, 0, 0, 0, 0, time.UTC), + Name: "Coffee", + ISOCurrencyCode: "USD", + }, + }, + Cursor: "cursor_1", + }, + } + + providers := map[string]provider.Provider{"plaid": mock} + engine := newEngine(t, pool, queries, providers) + + if err := engine.Sync(ctx, conn.ID, db.SyncTriggerManual); err != nil { + t.Fatalf("Sync() error: %v", err) + } + + // The new account must now exist, linked to this connection. + var newAcctID, newAcctConn pgtype.UUID + var newAcctName string + if err := pool.QueryRow(ctx, + "SELECT id, connection_id, name FROM accounts WHERE external_account_id = $1", + "ext_acct_2").Scan(&newAcctID, &newAcctConn, &newAcctName); err != nil { + t.Fatalf("new account was not created during sync: %v", err) + } + if newAcctConn != conn.ID { + t.Errorf("new account linked to wrong connection: got %v want %v", newAcctConn, conn.ID) + } + if newAcctName != "New Savings" { + t.Errorf("new account name = %q, want %q", newAcctName, "New Savings") + } + + // Its transaction must have resolved onto the new account rather than being dropped. + var txnCount int + if err := pool.QueryRow(ctx, + "SELECT count(*) FROM transactions WHERE account_id = $1 AND deleted_at IS NULL", + newAcctID).Scan(&txnCount); err != nil { + t.Fatalf("count txns on new account: %v", err) + } + if txnCount != 1 { + t.Errorf("expected 1 transaction on the newly discovered account, got %d", txnCount) + } + + // The pre-existing account must be left intact — discovery must skip accounts + // already linked to the connection, not duplicate or re-create them. + var existingCount int + if err := pool.QueryRow(ctx, + "SELECT count(*) FROM accounts WHERE external_account_id = $1 AND connection_id = $2", + "ext_acct_1", conn.ID).Scan(&existingCount); err != nil { + t.Fatalf("count existing account rows: %v", err) + } + if existingCount != 1 { + t.Errorf("expected exactly 1 row for the pre-existing account, got %d (discovery duplicated it)", existingCount) + } +} + func TestSync_ModifyTransactions(t *testing.T) { pool, queries := testutil.ServicePool(t) ctx := context.Background()