diff --git a/docs/bug-fixes.md b/docs/bug-fixes.md index cf346e9..b94ebb3 100644 --- a/docs/bug-fixes.md +++ b/docs/bug-fixes.md @@ -54,6 +54,52 @@ The Return Center reports only the refund total across all payment methods ($42. **Commit:** See PR. +### 2026-10-12: `-order-id` syncs could claim another order's Monarch charge + +**Description:** +While checking the exchanged order in the next entry, an `-order-id` dry run for it matched the replacement order's $34.42 charge, which that order had already itemized, alongside the order's own $58.11. A real run would have overwritten the replacement charge's notes and category with the original order's items, scaled down to $34.42. + +**Test Cases:** +- `TestReserveClaimedTransactions_OrderIDRunReservesOtherOrdersCharges` (`internal/application/sync/order_filter_test.go`) +- `TestReserveClaimedTransactions_FullRunLeavesReservationToReconciliation` +- `TestStorage_ClaimedTransactionIDs`, `TestMockRepository_ClaimedTransactionIDs` + +**Root Cause:** +In a full sync, each stored order reserves its Monarch transaction in `usedTransactionIDs` as `reconcileStoredOrder` resolves it. With `-order-id`, the loop skips every other order before that step, so nothing is reserved and the one order can match any charge in the window. + +**Fix Applied:** +- New `OrderRepository.ClaimedTransactionIDs(excludeOrderID)` returns the transaction IDs held by other synced orders (status `success` or `provisional`, not dry runs): each record's transaction plus every transaction a real run recorded for it in `order_transactions`, so all charges of a multi-charge order are included. +- `Run` reserves them before processing when `-order-id` is set. Full syncs are unchanged. + +**Verification:** +- The reservation test fails before the fix (the helper didn't exist) and passes after it. With the fix, the `-order-id` dry run matched only the $58.11 charge. +- `go test -race ./...` and `golangci-lint run` (v2.14.0) pass. + +**Commit:** See the pull request. + +### 2026-10-12: Monarch-side discovery missed an order's own charge on busy accounts + +**Description:** +A $58.11 Amazon order was skipped on every sync with "bank charges ($92.53) exceed expected ($58.11) by $34.42 - possible duplicate or extra charge". Its mattress pad had been exchanged, and Amazon lists the replacement order's $34.42 charge under the original order as well. The replacement order had already claimed that charge, so the handler fell back to Monarch-side discovery, which should have found the order's own $58.11 charge on its own. It never did. + +**Test Cases:** +- `TestAmazonHandler_ProcessOrder_ExchangeReplacementChargeListedUnderOriginal` +- `TestFindSubsetByTotalIncluding_IgnoresChargesLargerThanTotal` (`internal/domain/matcher/subset_test.go`) +- `TestFindSubsetByTotalIncluding_PrefersChargesClosestToOrderDate` (`internal/domain/matcher/subset_test.go`) + +**Root Cause:** +`subsetSummingTo` searches at most the first 20 candidates. `FindSubsetByTotalIncluding` passed the candidates in Monarch's order, newest first, so on an account with many Amazon charges inside the discovery window the order's own charges could fall past the cutoff. The account in question had 56 Amazon charges in the window, and the $58.11 was candidate 33. + +**Fix Applied:** +- Candidates larger than the order total are left out, since they can't be part of a subset summing to it. +- The remaining candidates are ordered by distance from the order date (stable sort) before the 20-candidate cap. + +**Verification:** +- All three tests fail before the fix and pass after it. +- `go test -race ./...` and `golangci-lint run` (v2.14.0) pass. + +**Commit:** See the pull request. + ### 2026-10-03: Amazon multi-category splits added allocated tax twice **Description:** diff --git a/internal/application/sync/handlers/amazon_test.go b/internal/application/sync/handlers/amazon_test.go index c0437d9..08d2efa 100644 --- a/internal/application/sync/handlers/amazon_test.go +++ b/internal/application/sync/handlers/amazon_test.go @@ -3,6 +3,7 @@ package handlers import ( "context" "errors" + "fmt" "testing" "time" @@ -1071,3 +1072,48 @@ func TestAllocatedAmazonOrderSplitsDoNotAddTaxTwice(t *testing.T) { assert.Equal(t, -148.84, amountsByCategory["home"]+amountsByCategory["clothing"]) assert.Contains(t, notesByCategory["home"], "$73.65") } + +// Regression: a $58.11 order whose mattress pad was exchanged. Amazon lists +// the replacement order's $34.42 charge under the original order too. The +// replacement order had already claimed that charge, so Monarch-side discovery +// had to find the $58.11 on its own, but it only searched the first 20 of the +// account's Amazon charges (newest first). +func TestAmazonHandler_ProcessOrder_ExchangeReplacementChargeListedUnderOriginal(t *testing.T) { + orderDate := time.Date(2026, 9, 13, 0, 0, 0, 0, time.UTC) + order := &mockAmazonOrder{ + id: "112-1111111-1111111", + date: orderDate, + total: 58.11, + subtotal: 53.93, + tax: 4.18, + items: []providers.OrderItem{ + &mockItem{name: "Shower Curtain Rod", price: 21.99}, + &mockItem{name: "Twin Mattress Pad", price: 31.94}, + }, + bankCharges: []float64{34.42, 58.11}, + } + var monarchTxns []*monarch.Transaction + for i := 0; i < 30; i++ { + monarchTxns = append(monarchTxns, &monarch.Transaction{ + ID: fmt.Sprintf("newer-%d", i), Amount: -12.00 - float64(i), Date: toMonarchDate(orderDate.AddDate(0, 0, 9-i%9)), + }) + } + monarchTxns = append(monarchTxns, + &monarch.Transaction{ID: "replacement-charge", Amount: -34.42, Date: toMonarchDate(orderDate.AddDate(0, 0, 2))}, + &monarch.Transaction{ID: "pad-refund", Amount: 34.42, Date: toMonarchDate(orderDate.AddDate(0, 0, 1))}, + &monarch.Transaction{ID: "own-charge", Amount: -58.11, Date: toMonarchDate(orderDate)}, + ) + monarchClient := &mockMonarch{} + handler := NewAmazonHandler( + matcher.NewMatcher(matcher.DefaultConfig()), + &mockConsolidator{}, &mockSplitter{categoryID: "home", notes: "Home & Garden"}, monarchClient, nil, + ) + usedTxnIDs := map[string]bool{"replacement-charge": true} + + result, err := handler.ProcessOrder(context.Background(), order, monarchTxns, usedTxnIDs, nil, nil, false) + + require.NoError(t, err) + require.True(t, result.Processed, "skip reason: %s", result.SkipReason) + assert.Equal(t, []string{"own-charge"}, monarchClient.updatedIDs) + assert.InDelta(t, 58.11, result.Allocations.TotalAllocated, 0.001) +} diff --git a/internal/application/sync/orchestrator.go b/internal/application/sync/orchestrator.go index ca7b50b..45b34cc 100644 --- a/internal/application/sync/orchestrator.go +++ b/internal/application/sync/orchestrator.go @@ -115,6 +115,27 @@ func (o *Orchestrator) processOrder( return false, true, nil } +// reserveClaimedTransactions marks the Monarch transactions that other synced +// orders own as used before an -order-id run. A full run reserves them as it +// reconciles each stored order, but -order-id leaves the other orders out, so +// the one order could otherwise claim (and overwrite) another order's charge, +// e.g. an exchange's replacement charge that Amazon also lists under the +// original order. +func (o *Orchestrator) reserveClaimedTransactions(opts Options, used map[string]bool) error { + if opts.OrderID == "" || o.storage == nil { + return nil + } + claimed, err := o.storage.ClaimedTransactionIDs(opts.OrderID) + if err != nil { + return fmt.Errorf("load transactions claimed by other orders: %w", err) + } + for id := range claimed { + used[id] = true + } + o.logger.Debug("Reserved transactions claimed by other orders", "count", len(claimed)) + return nil +} + // Run executes the sync process for the configured provider func (o *Orchestrator) Run(ctx context.Context, opts Options) (*Result, error) { result := &Result{ @@ -182,6 +203,10 @@ func (o *Orchestrator) Run(ctx context.Context, opts Options) (*Result, error) { // 5. Process orders usedTransactionIDs := make(map[string]bool) + if err := o.reserveClaimedTransactions(opts, usedTransactionIDs); err != nil { + o.completeFailedRun(1) + return nil, err + } for i, order := range orders { if opts.OrderID != "" && order.GetID() != opts.OrderID { diff --git a/internal/application/sync/order_filter_test.go b/internal/application/sync/order_filter_test.go new file mode 100644 index 0000000..b22dc74 --- /dev/null +++ b/internal/application/sync/order_filter_test.go @@ -0,0 +1,42 @@ +package sync + +import ( + "testing" + + "github.com/eshaffer321/itemize/internal/infrastructure/storage" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Regression: an -order-id dry run for an order with an exchanged item matched +// the $34.42 charge that the exchange's replacement order had already +// itemized, because orders left out by -order-id never reserve their +// transactions. A real run would have overwritten that charge's notes. +func TestReserveClaimedTransactions_OrderIDRunReservesOtherOrdersCharges(t *testing.T) { + store := storage.NewMockRepository() + require.NoError(t, store.SaveRecord(&storage.ProcessingRecord{ + OrderID: "112-2222222-2222222", Status: "success", TransactionID: "replacement-charge", + })) + require.NoError(t, store.SaveRecord(&storage.ProcessingRecord{ + OrderID: "112-1111111-1111111", Status: "success", TransactionID: "own-charge", + })) + orch := &Orchestrator{storage: store, logger: reconciliationTestLogger()} + used := make(map[string]bool) + + require.NoError(t, orch.reserveClaimedTransactions(Options{OrderID: "112-1111111-1111111"}, used)) + + assert.Equal(t, map[string]bool{"replacement-charge": true}, used) +} + +func TestReserveClaimedTransactions_FullRunLeavesReservationToReconciliation(t *testing.T) { + store := storage.NewMockRepository() + require.NoError(t, store.SaveRecord(&storage.ProcessingRecord{ + OrderID: "112-2222222-2222222", Status: "success", TransactionID: "replacement-charge", + })) + orch := &Orchestrator{storage: store, logger: reconciliationTestLogger()} + used := make(map[string]bool) + + require.NoError(t, orch.reserveClaimedTransactions(Options{}, used)) + + assert.Empty(t, used) +} diff --git a/internal/domain/matcher/subset.go b/internal/domain/matcher/subset.go index 56b34c9..674cf2d 100644 --- a/internal/domain/matcher/subset.go +++ b/internal/domain/matcher/subset.go @@ -3,6 +3,7 @@ package matcher import ( "fmt" "math" + "sort" "github.com/eshaffer321/itemize/internal/adapters/providers" "github.com/eshaffer321/monarch-go/v2/pkg/monarch" @@ -59,9 +60,21 @@ func (m *Matcher) FindSubsetByTotalIncluding( if days > subsetDateTolerance { continue } + // A charge larger than the total can't be part of a subset summing to it. + if -txn.Amount > target+m.config.AmountTolerance { + continue + } candidates = append(candidates, txn) } + // The search looks at no more than 20 candidates. Monarch lists newest + // first, so on a busy account the order's own charges could fall past the + // cutoff; keep the ones closest to the order date. + sort.SliceStable(candidates, func(i, j int) bool { + return math.Abs(candidates[i].Date.Time.Sub(orderDate).Hours()) < + math.Abs(candidates[j].Date.Time.Sub(orderDate).Hours()) + }) + // Brute-force subset search — n is always small (typically 1–5) var accept func([]*monarch.Transaction) bool if len(requiredAmounts) > 0 { diff --git a/internal/domain/matcher/subset_test.go b/internal/domain/matcher/subset_test.go index 9e74b5d..fbd24fb 100644 --- a/internal/domain/matcher/subset_test.go +++ b/internal/domain/matcher/subset_test.go @@ -1,6 +1,7 @@ package matcher import ( + "fmt" "testing" "time" @@ -56,3 +57,44 @@ func TestFindSubsetByTotalIncluding_AcceptsSubsetContainingOrderCharge(t *testin require.Len(t, matches, 2) assert.ElementsMatch(t, []string{"known", "late"}, []string{matches[0].ID, matches[1].ID}) } + +// Regression: a busy account had 56 Amazon charges inside the discovery +// window. Monarch lists newest first, so the order's own $58.11 was candidate +// 33 and the search only looked at the first 20. +func TestFindSubsetByTotalIncluding_IgnoresChargesLargerThanTotal(t *testing.T) { + orderDate := time.Date(2026, 9, 13, 0, 0, 0, 0, time.UTC) + order := &mockOrder{id: "112-1111111-1111111", date: orderDate, total: 58.11} + var transactions []*monarch.Transaction + for i := 0; i < 25; i++ { + transactions = append(transactions, &monarch.Transaction{ + ID: fmt.Sprintf("bigger-%d", i), Amount: -100 - float64(i), Date: monarch.Date{Time: orderDate.AddDate(0, 0, 1+i%9)}, + }) + } + transactions = append(transactions, &monarch.Transaction{ID: "own", Amount: -58.11, Date: monarch.Date{Time: orderDate}}) + m := NewMatcher(DefaultConfig()) + + matches, err := m.FindSubsetByTotalIncluding(order, transactions, map[string]bool{}, []float64{58.11}) + + require.NoError(t, err) + require.Len(t, matches, 1) + assert.Equal(t, "own", matches[0].ID) +} + +func TestFindSubsetByTotalIncluding_PrefersChargesClosestToOrderDate(t *testing.T) { + orderDate := time.Date(2026, 9, 13, 0, 0, 0, 0, time.UTC) + order := &mockOrder{id: "112-1111111-1111111", date: orderDate, total: 58.11} + var transactions []*monarch.Transaction + for i := 0; i < 25; i++ { + transactions = append(transactions, &monarch.Transaction{ + ID: fmt.Sprintf("later-%d", i), Amount: -7.00, Date: monarch.Date{Time: orderDate.AddDate(0, 0, 9-i%9)}, + }) + } + transactions = append(transactions, &monarch.Transaction{ID: "own", Amount: -58.11, Date: monarch.Date{Time: orderDate}}) + m := NewMatcher(DefaultConfig()) + + matches, err := m.FindSubsetByTotalIncluding(order, transactions, map[string]bool{}, []float64{58.11}) + + require.NoError(t, err) + require.Len(t, matches, 1) + assert.Equal(t, "own", matches[0].ID) +} diff --git a/internal/infrastructure/storage/interfaces.go b/internal/infrastructure/storage/interfaces.go index 7669bd7..0f121fd 100644 --- a/internal/infrastructure/storage/interfaces.go +++ b/internal/infrastructure/storage/interfaces.go @@ -31,6 +31,11 @@ type OrderRepository interface { // IsProcessed checks if an order has been successfully processed (non-dry-run) IsProcessed(orderID string) bool + // ClaimedTransactionIDs returns the Monarch transaction IDs that other + // synced orders (success or provisional, non-dry-run) already claimed, + // leaving out excludeOrderID's own. + ClaimedTransactionIDs(excludeOrderID string) (map[string]bool, error) + // GetStats returns aggregate statistics GetStats() (*Stats, error) diff --git a/internal/infrastructure/storage/mock.go b/internal/infrastructure/storage/mock.go index 568b205..fbc8a23 100644 --- a/internal/infrastructure/storage/mock.go +++ b/internal/infrastructure/storage/mock.go @@ -141,6 +141,27 @@ func (m *MockRepository) IsProcessed(orderID string) bool { return record.Status == "success" && !record.DryRun } +// ClaimedTransactionIDs returns transaction IDs of other synced orders' records +// and their real-run order transactions. +func (m *MockRepository) ClaimedTransactionIDs(excludeOrderID string) (map[string]bool, error) { + claimed := make(map[string]bool) + for orderID, record := range m.records { + if orderID == excludeOrderID || record.DryRun || + (record.Status != "success" && record.Status != "provisional") { + continue + } + if record.TransactionID != "" { + claimed[record.TransactionID] = true + } + for _, txn := range m.orderTxns[orderID] { + if run, ok := m.syncRuns[txn.RunID]; ok && !run.dryRun { + claimed[txn.TransactionID] = true + } + } + } + return claimed, nil +} + // GetStats returns mock statistics func (m *MockRepository) GetStats() (*Stats, error) { stats := &Stats{ diff --git a/internal/infrastructure/storage/sqlite.go b/internal/infrastructure/storage/sqlite.go index 2a5872a..4d543b5 100644 --- a/internal/infrastructure/storage/sqlite.go +++ b/internal/infrastructure/storage/sqlite.go @@ -677,6 +677,38 @@ func (s *Storage) IsProcessed(orderID string) bool { return err == nil && count > 0 } +// ClaimedTransactionIDs returns the Monarch transaction IDs claimed by other +// synced orders: each record's main transaction plus every transaction a real +// run associated with it (all charges of a multi-charge order). +func (s *Storage) ClaimedTransactionIDs(excludeOrderID string) (map[string]bool, error) { + query := ` + SELECT transaction_id FROM processing_records + WHERE dry_run = 0 AND status IN ('success', 'provisional') + AND order_id != ? AND COALESCE(transaction_id, '') != '' + UNION + SELECT ot.transaction_id FROM order_transactions ot + JOIN sync_runs sr ON sr.id = ot.run_id + JOIN processing_records pr ON pr.order_id = ot.order_id + WHERE sr.dry_run = 0 AND pr.dry_run = 0 AND pr.status IN ('success', 'provisional') + AND ot.order_id != ? + ` + rows, err := s.db.Query(query, excludeOrderID, excludeOrderID) + if err != nil { + return nil, err + } + defer func() { _ = rows.Close() }() + + claimed := make(map[string]bool) + for rows.Next() { + var id string + if err := rows.Scan(&id); err != nil { + return nil, err + } + claimed[id] = true + } + return claimed, rows.Err() +} + // StartSyncRun records the start of a sync run func (s *Storage) StartSyncRun(provider string, lookbackDays int, dryRun bool) (int64, error) { query := ` diff --git a/internal/infrastructure/storage/storage_test.go b/internal/infrastructure/storage/storage_test.go index f70db10..97d02b7 100644 --- a/internal/infrastructure/storage/storage_test.go +++ b/internal/infrastructure/storage/storage_test.go @@ -1156,3 +1156,61 @@ func TestStorage_AuditColumns_UpdateExisting(t *testing.T) { assert.Equal(t, "Personal Care:\n- Shampoo $8.99", retrieved.MonarchNotes) assert.Equal(t, `{"orderId":"walmart-456"}`, retrieved.RawOrderJSON) } + +func TestStorage_ClaimedTransactionIDs(t *testing.T) { + tmpDB := createTempDB(t) + defer os.Remove(tmpDB) + + store, err := NewStorage(tmpDB) + require.NoError(t, err) + defer store.Close() + + realRun, err := store.StartSyncRun("Amazon", 30, false) + require.NoError(t, err) + dryRun, err := store.StartSyncRun("Amazon", 30, true) + require.NoError(t, err) + + for _, r := range []*ProcessingRecord{ + {OrderID: "replacement", Status: "success", TransactionID: "txn-replacement"}, + {OrderID: "multi", Status: "success", TransactionID: "txn-multi-1"}, + {OrderID: "pending", Status: "provisional", TransactionID: "txn-pending"}, + {OrderID: "failed", Status: "failed", TransactionID: "txn-failed"}, + {OrderID: "preview", Status: "dry-run", DryRun: true, TransactionID: "txn-preview"}, + {OrderID: "target", Status: "success", TransactionID: "txn-own"}, + } { + r.Provider = "Amazon" + r.OrderDate = time.Now() + require.NoError(t, store.SaveRecord(r)) + } + require.NoError(t, store.SaveOrderTransaction(&OrderTransaction{RunID: realRun, OrderID: "multi", TransactionID: "txn-multi-2", Role: "matched"})) + require.NoError(t, store.SaveOrderTransaction(&OrderTransaction{RunID: dryRun, OrderID: "multi", TransactionID: "txn-multi-preview", Role: "matched"})) + require.NoError(t, store.SaveOrderTransaction(&OrderTransaction{RunID: realRun, OrderID: "failed", TransactionID: "txn-failed-2", Role: "matched"})) + require.NoError(t, store.SaveOrderTransaction(&OrderTransaction{RunID: realRun, OrderID: "target", TransactionID: "txn-own-2", Role: "matched"})) + + claimed, err := store.ClaimedTransactionIDs("target") + require.NoError(t, err) + + assert.Equal(t, map[string]bool{ + "txn-replacement": true, + "txn-multi-1": true, + "txn-multi-2": true, + "txn-pending": true, + }, claimed) +} + +func TestMockRepository_ClaimedTransactionIDs(t *testing.T) { + store := NewMockRepository() + realRun, _ := store.StartSyncRun("Amazon", 30, false) + dryRun, _ := store.StartSyncRun("Amazon", 30, true) + require.NoError(t, store.SaveRecord(&ProcessingRecord{OrderID: "replacement", Status: "success", TransactionID: "txn-replacement"})) + require.NoError(t, store.SaveRecord(&ProcessingRecord{OrderID: "multi", Status: "provisional", TransactionID: "txn-multi-1"})) + require.NoError(t, store.SaveRecord(&ProcessingRecord{OrderID: "preview", Status: "dry-run", DryRun: true, TransactionID: "txn-preview"})) + require.NoError(t, store.SaveRecord(&ProcessingRecord{OrderID: "target", Status: "success", TransactionID: "txn-own"})) + require.NoError(t, store.SaveOrderTransaction(&OrderTransaction{RunID: realRun, OrderID: "multi", TransactionID: "txn-multi-2"})) + require.NoError(t, store.SaveOrderTransaction(&OrderTransaction{RunID: dryRun, OrderID: "multi", TransactionID: "txn-multi-preview"})) + + claimed, err := store.ClaimedTransactionIDs("target") + require.NoError(t, err) + + assert.Equal(t, map[string]bool{"txn-replacement": true, "txn-multi-1": true, "txn-multi-2": true}, claimed) +}