fix: Amazon discovery missed charges past the 20th; -order-id could claim other orders' charges

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:**

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)
+}

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 {

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)
+}

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 {

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)
+}

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)
 

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{

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 := `