diff --git a/docs/bug-fixes.md b/docs/bug-fixes.md index a2d47e3..d8f7830 100644 --- a/docs/bug-fixes.md +++ b/docs/bug-fixes.md @@ -12,6 +12,35 @@ Each bug fix entry should include: ## Bug Fixes +### 2026-09-26: Amazon orders misreported, missed, or matched to other orders' charges + +**Description:** +An Amazon sync reported two errors: +- `112-3421348-5157859` was skipped with "could not find all transactions: expected 2, found 2". Only the $19.48 of its $13.77 + $19.48 charges had posted to Monarch. +- `112-7815140-3755432` (order total $55.31) was skipped because Amazon charged the card $59.36; a $4.05 discount on the order was not applied to the charge. The exact $59.36 transaction was in Monarch. + +When `112-7815140-3755432` was synced on its own, Monarch-side discovery matched $25.44 + $29.87, two charges belonging to other orders that happened to sum to $55.31. + +**Test Cases:** +- `TestAmazonHandler_ProcessOrder_PartialMultiChargeMatchIsPending` +- `TestAmazonHandler_ProcessOrder_NoMultiChargeMatchesReportsActualFoundCount` +- `TestAmazonHandler_ProcessOrder_OverchargeMatchesReportedCardCharge` +- `TestAmazonHandler_ProcessOrder_DiscoveryIgnoresSubsetsWithoutOrderCharge` +- `TestFindSubsetByTotalIncluding_RejectsSubsetWithoutOrderCharge` (`internal/domain/matcher/subset_test.go`) + +**Root Cause:** +- `FindMultipleMatches` keeps `nil` placeholders for unmatched charges, and the Amazon handler counted `len(Matches)` as the number found. +- A partial multi-charge match was reported as an error instead of waiting for the remaining charges to post. +- When charges failed validation, the handler went straight to discovery by order total and never tried the card charges Amazon reported for the order. +- Discovery accepted any Monarch subset summing to the order total, so the only guard against using other orders' charges was the processing order of `usedTxnIDs`. + +**Fix Applied:** +- The Amazon handler counts only non-nil matches, and a partial multi-charge match is skipped as `payment pending`. +- When card charges meet or exceed the expected amount, they are matched against Monarch directly and allocated using the charged amount. +- Discovery uses `FindSubsetByTotalIncluding`, which requires the subset to contain at least one charge Amazon reported for the order. + +**Commit:** Included in the pull request for this fix. + ### 2026-09-23: GPT-6 models would be sent `temperature` instead of `reasoning_effort` **Description:** diff --git a/internal/application/sync/handlers/amazon.go b/internal/application/sync/handlers/amazon.go index 0a71508..eaad71b 100644 --- a/internal/application/sync/handlers/amazon.go +++ b/internal/application/sync/handlers/amazon.go @@ -196,16 +196,83 @@ func (h *AmazonHandler) ProcessOrder( var consolidatedTxn *monarch.Transaction monarchDiscovered := false // true when we matched via subset search rather than provider charges - if !validation.Valid { - // Provider charges are incomplete (common for multi-shipment orders where later - // charges post after the provider visited the order details page). Try to find the - // matching Monarch transactions by searching for a subset that sums to the order total. + // Charges that meet or exceed the expected amount fully cover the order, so + // the card charges Amazon attributes to it are the ground truth for what + // hit the bank. Overcharges happen when a discount shown on the order is + // not applied to the card charge. + chargesCoverOrder := validation.Valid || validation.Difference > 0 + var reportedTxns []*monarch.Transaction + reportedFound := 0 + if chargesCoverOrder { + reportedTxns, reportedFound, err = h.matchReportedCharges(order, monarchTxns, usedTxnIDs, bankCharges) + if err != nil { + return nil, err + } + } + + switch { + case reportedTxns != nil: + matchedTxns = reportedTxns + for _, t := range matchedTxns { + usedTxnIDs[t.ID] = true + } + if validation.Valid { + h.logDebug("Matched reported charges", + "order_id", order.GetID(), + "transaction_count", len(matchedTxns)) + } else { + h.logInfo("Matched reported card charges that differ from order total", + "order_id", order.GetID(), + "bank_sum", validation.BankChargesSum, + "expected", validation.ExpectedSum, + "difference", validation.Difference) + } + if len(matchedTxns) > 1 { + h.logInfo("Matched all transactions for multi-delivery order", + "order_id", order.GetID(), + "transaction_count", len(matchedTxns)) + } + + case validation.Valid && len(bankCharges) == 1: + result.Skipped = true + result.SkipReason = "no matching transaction found" + h.logWarn("No matching transaction found", + "order_id", order.GetID(), + "expected_amount", bankCharges[0]) + return result, nil + + case validation.Valid && reportedFound > 0: + // Some of the order's charges are in Monarch; the rest have not posted yet. + result.Skipped = true + result.SkipReason = "payment pending" + h.logInfo("Waiting for remaining Amazon charges to post", + "order_id", order.GetID(), + "expected", len(bankCharges), + "found", reportedFound) + return result, nil + + case validation.Valid: + result.Skipped = true + result.SkipReason = fmt.Sprintf("could not find all transactions: expected %d, found %d", + len(bankCharges), reportedFound) + h.logWarn("Not all transactions found", + "order_id", order.GetID(), + "expected", len(bankCharges), + "found", reportedFound) + return result, nil + + default: + // Provider charges don't reconcile with the order total (common for + // multi-shipment orders where later charges post after the provider + // visited the order details page). Search Monarch for a subset that sums + // to the order total and includes at least one charge Amazon reported + // for this order, so unrelated orders' charges can't fill the total. h.logDebug("Provider charges incomplete, attempting Monarch-side discovery", "order_id", order.GetID(), "provider_charge_sum", validation.BankChargesSum, "expected", validation.ExpectedSum) - discovered, discoverErr := h.matcher.FindSubsetByTotal(order, monarchTxns, usedTxnIDs) + discovered, discoverErr := h.matcher.FindSubsetByTotalIncluding(order, monarchTxns, usedTxnIDs, bankCharges) if discoverErr != nil { h.logWarn("Charge validation failed and Monarch discovery found no match", "order_id", order.GetID(), @@ -226,68 +293,6 @@ func (h *AmazonHandler) ProcessOrder( h.logInfo("Monarch-side discovery found matching transactions", "order_id", order.GetID(), "count", len(matchedTxns)) - } else { - h.logDebug("Charge validation passed", - "order_id", order.GetID(), - "bank_sum", validation.BankChargesSum, - "expected", validation.ExpectedSum) - - if len(bankCharges) > 1 { - // Multi-delivery order - find multiple matches - multiResult, err := h.matcher.FindMultipleMatches(order, monarchTxns, usedTxnIDs, bankCharges) - if err != nil { - return nil, fmt.Errorf("multi-match error: %w", err) - } - - if !multiResult.AllFound { - result.Skipped = true - result.SkipReason = fmt.Sprintf("could not find all transactions: expected %d, found %d", - len(bankCharges), len(multiResult.Matches)) - h.logWarn("Not all transactions found", - "order_id", order.GetID(), - "expected", len(bankCharges), - "found", len(multiResult.Matches)) - return result, nil - } - - for _, match := range multiResult.Matches { - matchedTxns = append(matchedTxns, match.Transaction) - usedTxnIDs[match.Transaction.ID] = true - } - h.logInfo("Matched all transactions for multi-delivery order", - "order_id", order.GetID(), - "transaction_count", len(matchedTxns)) - } else { - // Single charge - find one match - // Use a wrapper order that returns the bank charge amount for matching - // This handles gift card orders where order total differs from bank charge - matchOrder := &bankChargeOrder{ - Order: order, - bankCharge: bankCharges[0], - } - - matchResult, err := h.matcher.FindMatch(matchOrder, monarchTxns, usedTxnIDs) - if err != nil { - return nil, fmt.Errorf("match error: %w", err) - } - - if matchResult == nil { - result.Skipped = true - result.SkipReason = "no matching transaction found" - h.logWarn("No matching transaction found", - "order_id", order.GetID(), - "expected_amount", bankCharges[0]) - return result, nil - } - - consolidatedTxn = matchResult.Transaction - usedTxnIDs[consolidatedTxn.ID] = true - - h.logDebug("Matched single transaction", - "order_id", order.GetID(), - "transaction_id", consolidatedTxn.ID, - "amount", math.Abs(consolidatedTxn.Amount)) - } } // Never consolidate multiple pending bank-feed rows. Pending transactions @@ -457,6 +462,51 @@ func (h *AmazonHandler) ProcessOrder( return result, nil } +// matchReportedCharges finds a Monarch transaction for each card charge Amazon +// reported for the order. It returns the matches only when every charge was +// found, along with how many were found, and does not mark anything as used. +func (h *AmazonHandler) matchReportedCharges( + order AmazonOrder, + monarchTxns []*monarch.Transaction, + usedTxnIDs map[string]bool, + bankCharges []float64, +) ([]*monarch.Transaction, int, error) { + if len(bankCharges) == 1 { + // Match on the bank charge rather than the order total, which can differ + // for gift card, points, and discount adjustments. + matchOrder := &bankChargeOrder{ + Order: order, + bankCharge: bankCharges[0], + } + matchResult, err := h.matcher.FindMatch(matchOrder, monarchTxns, usedTxnIDs) + if err != nil { + return nil, 0, fmt.Errorf("match error: %w", err) + } + if matchResult == nil { + return nil, 0, nil + } + h.logDebug("Matched single transaction", + "order_id", order.GetID(), + "transaction_id", matchResult.Transaction.ID, + "amount", math.Abs(matchResult.Transaction.Amount)) + return []*monarch.Transaction{matchResult.Transaction}, 1, nil + } + + multiResult, err := h.matcher.FindMultipleMatches(order, monarchTxns, usedTxnIDs, bankCharges) + if err != nil { + return nil, 0, fmt.Errorf("multi-match error: %w", err) + } + found := countFoundMatches(multiResult.Matches) + if !multiResult.AllFound { + return nil, found, nil + } + matched := make([]*monarch.Transaction, 0, len(multiResult.Matches)) + for _, match := range multiResult.Matches { + matched = append(matched, match.Transaction) + } + return matched, found, nil +} + // bankChargeOrder wraps an order to return the bank charge amount for matching // This handles gift card orders where the order total differs from the bank charge type bankChargeOrder struct { diff --git a/internal/application/sync/handlers/amazon_test.go b/internal/application/sync/handlers/amazon_test.go index ba9c3d4..accfdf0 100644 --- a/internal/application/sync/handlers/amazon_test.go +++ b/internal/application/sync/handlers/amazon_test.go @@ -278,7 +278,130 @@ func TestAmazonHandler_ProcessOrder_MissingTransactions(t *testing.T) { require.NoError(t, err) assert.True(t, result.Skipped) - assert.Contains(t, result.SkipReason, "could not find all transactions") + // One of two charges matched, so the other has not posted yet. + assert.Equal(t, "payment pending", result.SkipReason) +} + +// Regression: order 112-3421348-5157859 split into $13.77 + $19.48 card charges. +// Only $19.48 had posted to Monarch. The skip was reported as an error saying +// "expected 2, found 2" because nil placeholders were counted as matches. +func TestAmazonHandler_ProcessOrder_PartialMultiChargeMatchIsPending(t *testing.T) { + orderDate := time.Now() + order := &mockAmazonOrder{ + id: "112-3421348-5157859", + date: orderDate, + total: 33.25, + items: []providers.OrderItem{&mockItem{name: "Item A", price: 14.00}, &mockItem{name: "Item B", price: 19.25}}, + bankCharges: []float64{13.77, 19.48}, + nonBankAmount: 1.50, // Visa points earned, not a payment + } + monarchTxns := []*monarch.Transaction{ + {ID: "posted", Amount: -19.48, Date: toMonarchDate(orderDate), Pending: true}, + } + splitter := &mockSplitter{} + monarchClient := &mockMonarch{} + handler := NewAmazonHandler( + matcher.NewMatcher(matcher.Config{AmountTolerance: 0.01, DateTolerance: 5}), + &mockConsolidator{}, splitter, monarchClient, nil, + ) + usedTxnIDs := make(map[string]bool) + + result, err := handler.ProcessOrder(context.Background(), order, monarchTxns, usedTxnIDs, nil, nil, false) + + require.NoError(t, err) + assert.True(t, result.Skipped) + assert.Equal(t, "payment pending", result.SkipReason) + assert.False(t, usedTxnIDs["posted"], "a pending order must not claim transactions") + assert.Nil(t, splitter.lastOrder) + assert.False(t, monarchClient.updateCalled) +} + +func TestAmazonHandler_ProcessOrder_NoMultiChargeMatchesReportsActualFoundCount(t *testing.T) { + order := &mockAmazonOrder{ + id: "no-charges-posted", + date: time.Now(), + total: 33.25, + items: []providers.OrderItem{&mockItem{name: "Item", price: 33.25}}, + bankCharges: []float64{13.77, 19.48}, + } + handler := NewAmazonHandler( + matcher.NewMatcher(matcher.Config{AmountTolerance: 0.01, DateTolerance: 5}), + nil, nil, nil, nil, + ) + + result, err := handler.ProcessOrder(context.Background(), order, nil, make(map[string]bool), nil, nil, false) + + require.NoError(t, err) + assert.True(t, result.Skipped) + assert.Equal(t, "could not find all transactions: expected 2, found 0", result.SkipReason) +} + +// Regression: order 112-7815140-3755432 had an order total of $55.31 but Amazon +// charged the card $59.36 (a $4.05 discount was not applied to the charge). +// The exact $59.36 transaction was in Monarch but was never tried. +func TestAmazonHandler_ProcessOrder_OverchargeMatchesReportedCardCharge(t *testing.T) { + orderDate := time.Date(2026, 9, 24, 0, 0, 0, 0, time.UTC) + order := &mockAmazonOrder{ + id: "112-7815140-3755432", + date: orderDate, + total: 55.31, + subtotal: 56.00, + tax: 3.36, + items: []providers.OrderItem{ + &mockItem{name: "Leggings", price: 32.00}, + &mockItem{name: "T-Shirt", price: 24.00}, + }, + bankCharges: []float64{59.36}, + } + monarchTxns := []*monarch.Transaction{ + {ID: "other-order-a", Amount: -25.44, Date: toMonarchDate(orderDate)}, + {ID: "other-order-b", Amount: -29.87, Date: toMonarchDate(orderDate)}, + {ID: "own-charge", Amount: -59.36, Date: toMonarchDate(orderDate.AddDate(0, 0, 1))}, + } + splitter := &mockSplitter{categoryID: "clothing", notes: "Clothing:\n- Leggings\n- T-Shirt"} + monarchClient := &mockMonarch{} + handler := NewAmazonHandler( + matcher.NewMatcher(matcher.Config{AmountTolerance: 0.01, DateTolerance: 5}), + &mockConsolidator{}, splitter, monarchClient, nil, + ) + usedTxnIDs := make(map[string]bool) + + 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, "own-charge", result.Transaction.ID) + assert.Equal(t, "own-charge", monarchClient.updatedID) + assert.InDelta(t, 59.36, result.Allocations.TotalAllocated, 0.001) + assert.False(t, usedTxnIDs["other-order-a"]) + assert.False(t, usedTxnIDs["other-order-b"]) +} + +func TestAmazonHandler_ProcessOrder_DiscoveryIgnoresSubsetsWithoutOrderCharge(t *testing.T) { + orderDate := time.Date(2026, 9, 24, 0, 0, 0, 0, time.UTC) + order := &mockAmazonOrder{ + id: "112-7815140-3755432", + date: orderDate, + total: 55.31, + items: []providers.OrderItem{&mockItem{name: "Leggings", price: 56.00}}, + bankCharges: []float64{59.36}, // not yet in Monarch + } + monarchTxns := []*monarch.Transaction{ + {ID: "other-order-a", Amount: -25.44, Date: toMonarchDate(orderDate)}, + {ID: "other-order-b", Amount: -29.87, Date: toMonarchDate(orderDate)}, + } + handler := NewAmazonHandler( + matcher.NewMatcher(matcher.Config{AmountTolerance: 0.01, DateTolerance: 5}), + &mockConsolidator{}, &mockSplitter{}, &mockMonarch{}, nil, + ) + usedTxnIDs := make(map[string]bool) + + result, err := handler.ProcessOrder(context.Background(), order, monarchTxns, usedTxnIDs, nil, nil, false) + + require.NoError(t, err) + assert.True(t, result.Skipped) + assert.Contains(t, result.SkipReason, "exceed expected") + assert.Empty(t, usedTxnIDs, "unrelated transactions must not be claimed") } func TestAmazonHandler_ProcessOrder_DoesNotConsolidatePendingMultiChargeTransactions(t *testing.T) { diff --git a/internal/domain/matcher/subset.go b/internal/domain/matcher/subset.go index b5eaa8e..56b34c9 100644 --- a/internal/domain/matcher/subset.go +++ b/internal/domain/matcher/subset.go @@ -24,6 +24,20 @@ func (m *Matcher) FindSubsetByTotal( order providers.Order, monarchTxns []*monarch.Transaction, usedTxnIDs map[string]bool, +) ([]*monarch.Transaction, error) { + return m.FindSubsetByTotalIncluding(order, monarchTxns, usedTxnIDs, nil) +} + +// FindSubsetByTotalIncluding is FindSubsetByTotal restricted to subsets that +// contain at least one transaction matching one of requiredAmounts. Callers pass +// the charges the provider already attributes to this order so that discovery +// cannot assemble the total purely from other orders' charges that happen to +// sum to it. A nil or empty requiredAmounts applies no restriction. +func (m *Matcher) FindSubsetByTotalIncluding( + order providers.Order, + monarchTxns []*monarch.Transaction, + usedTxnIDs map[string]bool, + requiredAmounts []float64, ) ([]*monarch.Transaction, error) { target := order.GetTotal() if target <= 0 { @@ -49,7 +63,13 @@ func (m *Matcher) FindSubsetByTotal( } // Brute-force subset search — n is always small (typically 1–5) - matches := subsetSummingTo(candidates, target, m.config.AmountTolerance) + var accept func([]*monarch.Transaction) bool + if len(requiredAmounts) > 0 { + accept = func(subset []*monarch.Transaction) bool { + return containsAnyAmount(subset, requiredAmounts, m.config.AmountTolerance) + } + } + matches := subsetSummingTo(candidates, target, m.config.AmountTolerance, accept) if matches == nil { return nil, fmt.Errorf("no combination of Monarch transactions sums to order total $%.2f", target) } @@ -57,9 +77,17 @@ func (m *Matcher) FindSubsetByTotal( } // subsetSummingTo returns the closest subset of txns whose absolute amounts -// sum to target within tolerance, or nil if none exists. When multiple subsets -// are equally close, it prefers the one with fewer transactions. -func subsetSummingTo(txns []*monarch.Transaction, target, tolerance float64) []*monarch.Transaction { +// sum to target within tolerance and that accept approves, or nil if none +// exists. A nil accept approves every subset. When multiple subsets are equally +// close, it prefers the one with fewer transactions. +func subsetSummingTo( + txns []*monarch.Transaction, + target, tolerance float64, + accept func([]*monarch.Transaction) bool, +) []*monarch.Transaction { + if accept == nil { + accept = func([]*monarch.Transaction) bool { return true } + } n := len(txns) if n > 20 { n = 20 // guard; 2^20 is ~1M — still fast, but cap for safety @@ -71,7 +99,7 @@ func subsetSummingTo(txns []*monarch.Transaction, target, tolerance float64) []* // Search every valid subset. Iterating by size preserves the fewer-transactions // tie-breaker while allowing an exact total to beat an earlier tolerated match. for size := 1; size <= n; size++ { - result, difference := closestSubsetOfSize(txns[:n], target, tolerance, 0, size, nil) + result, difference := closestSubsetOfSize(txns[:n], target, tolerance, accept, 0, size, nil) if result != nil && difference < bestDifference { best = result bestDifference = difference @@ -85,6 +113,7 @@ func subsetSummingTo(txns []*monarch.Transaction, target, tolerance float64) []* func closestSubsetOfSize( txns []*monarch.Transaction, target, tolerance float64, + accept func([]*monarch.Transaction) bool, start, remaining int, current []*monarch.Transaction, ) ([]*monarch.Transaction, float64) { @@ -94,7 +123,7 @@ func closestSubsetOfSize( sum += math.Abs(t.Amount) } difference := math.Abs(sum - target) - if difference <= tolerance { + if difference <= tolerance && accept(current) { result := make([]*monarch.Transaction, len(current)) copy(result, current) return result, difference @@ -105,7 +134,7 @@ func closestSubsetOfSize( var best []*monarch.Transaction bestDifference := math.Inf(1) for i := start; i <= len(txns)-remaining; i++ { - found, difference := closestSubsetOfSize(txns, target, tolerance, i+1, remaining-1, + found, difference := closestSubsetOfSize(txns, target, tolerance, accept, i+1, remaining-1, append(current, txns[i])) if found != nil && difference < bestDifference { best = found @@ -114,3 +143,17 @@ func closestSubsetOfSize( } return best, bestDifference } + +// containsAnyAmount reports whether any transaction's absolute amount is within +// tolerance of one of amounts. +func containsAnyAmount(txns []*monarch.Transaction, amounts []float64, tolerance float64) bool { + const epsilon = 0.0000001 + for _, t := range txns { + for _, amount := range amounts { + if math.Abs(math.Abs(t.Amount)-amount) <= tolerance+epsilon { + return true + } + } + } + return false +} diff --git a/internal/domain/matcher/subset_test.go b/internal/domain/matcher/subset_test.go index b1c5570..9e74b5d 100644 --- a/internal/domain/matcher/subset_test.go +++ b/internal/domain/matcher/subset_test.go @@ -2,6 +2,7 @@ package matcher import ( "testing" + "time" "github.com/eshaffer321/monarch-go/v2/pkg/monarch" "github.com/stretchr/testify/assert" @@ -14,8 +15,44 @@ func TestSubsetSummingTo_PrefersExactTotalOverSmallerToleratedSubset(t *testing. {ID: "penny", Amount: -0.01}, } - matches := subsetSummingTo(transactions, 10.00, 0.01) + matches := subsetSummingTo(transactions, 10.00, 0.01, nil) require.Len(t, matches, 2) assert.Equal(t, []string{"near", "penny"}, []string{matches[0].ID, matches[1].ID}) } + +// Regression: order 112-7815140-3755432 ($55.31 total, one $59.36 card charge). +// Unanchored discovery paired two unrelated charges from other orders +// ($25.44 + $29.87) because they happened to sum to the order total. +func TestFindSubsetByTotalIncluding_RejectsSubsetWithoutOrderCharge(t *testing.T) { + orderDate := time.Date(2026, 9, 24, 0, 0, 0, 0, time.UTC) + order := &mockOrder{id: "112-7815140-3755432", date: orderDate, total: 55.31} + transactions := []*monarch.Transaction{ + {ID: "own-charge", Amount: -59.36, Date: monarch.Date{Time: orderDate}}, + {ID: "other-order-a", Amount: -25.44, Date: monarch.Date{Time: orderDate}}, + {ID: "other-order-b", Amount: -29.87, Date: monarch.Date{Time: orderDate}}, + } + m := NewMatcher(DefaultConfig()) + + matches, err := m.FindSubsetByTotalIncluding(order, transactions, map[string]bool{}, []float64{59.36}) + + assert.Error(t, err) + assert.Nil(t, matches) +} + +func TestFindSubsetByTotalIncluding_AcceptsSubsetContainingOrderCharge(t *testing.T) { + orderDate := time.Date(2026, 9, 24, 0, 0, 0, 0, time.UTC) + order := &mockOrder{id: "multi-shipment", date: orderDate, total: 103.27} + transactions := []*monarch.Transaction{ + {ID: "known", Amount: -52.55, Date: monarch.Date{Time: orderDate}}, + {ID: "late", Amount: -50.72, Date: monarch.Date{Time: orderDate.AddDate(0, 0, 3)}}, + {ID: "unrelated", Amount: -12.00, Date: monarch.Date{Time: orderDate}}, + } + m := NewMatcher(DefaultConfig()) + + matches, err := m.FindSubsetByTotalIncluding(order, transactions, map[string]bool{}, []float64{52.55}) + + require.NoError(t, err) + require.Len(t, matches, 2) + assert.ElementsMatch(t, []string{"known", "late"}, []string{matches[0].ID, matches[1].ID}) +}