diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index df52a2b..d80de97 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -7,7 +7,7 @@ on: branches: [ main, develop ] env: - GO_VERSION: '1.25.12' + GO_VERSION: '1.25.13' jobs: lint: @@ -72,7 +72,7 @@ jobs: needs: [module-integrity] strategy: matrix: - go-version: ['1.25.12'] + go-version: ['1.25.13'] steps: - name: Checkout code uses: actions/checkout@v7 @@ -92,7 +92,7 @@ jobs: go tool cover -func=coverage.txt - name: Upload coverage reports to Codecov - if: matrix.go-version == '1.25.12' && (github.event_name != 'pull_request' || github.event.pull_request.user.login != 'dependabot[bot]') + if: matrix.go-version == '1.25.13' && (github.event_name != 'pull_request' || github.event.pull_request.user.login != 'dependabot[bot]') uses: codecov/codecov-action@v7 with: token: ${{ secrets.CODECOV_TOKEN }} diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index cf81ed0..5c81b3a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -10,7 +10,7 @@ permissions: packages: write env: - GO_VERSION: '1.25.12' + GO_VERSION: '1.25.13' jobs: test: diff --git a/docs/bug-fixes.md b/docs/bug-fixes.md index 98b9050..f97ae65 100644 --- a/docs/bug-fixes.md +++ b/docs/bug-fixes.md @@ -12,6 +12,46 @@ Each bug fix entry should include: ## Bug Fixes +### 2026-08-23: CI and release builds used a vulnerable Go patch release + +**Description:** +The pull-request vulnerability scan failed with five reachable standard-library vulnerabilities because CI and release builds were pinned to Go 1.25.12. All five are fixed in Go 1.25.13. + +**Fix Applied:** +Raised the module, CI, and release workflow toolchain to Go 1.25.13 so tests and published binaries use the patched standard library. + +**Verification:** +- `govulncheck` reports the affected standard-library symbols as fixed with Go 1.25.13. +- CI and GoReleaser now select Go 1.25.13. + +**Commit:** Included in the pull request for this fix. + +### 2026-08-23: Direct Monarch transaction updates did not retry transient failures + +**Description:** +An Amazon order matched and categorized successfully, but its final Monarch category-and-notes update received a `502 Bad Gateway`. Itemize recorded the order as failed after one request, leaving the posted transaction in its temporary Amazon category. Multi-delivery consolidation already retried transient Monarch update failures, but direct updates used by provider handlers and reconciliation did not. + +**Test Case:** +```go +// internal/application/sync/process_order_test.go: +// TestMonarchAdapter_UpdateTransactionRetriesRetryableError +// TestMonarchAdapter_UpdateTransactionDoesNotRetryPermanentError +``` + +**Root Cause:** +The shared `monarchAdapter.UpdateTransaction` method made exactly one API call. Although `monarch-go` classifies 5xx responses as retryable, the adapter never consulted that classification, and Itemize intentionally does not enable blanket client-level retries for every GraphQL operation. + +**Fix Applied:** +The shared Itemize transaction-update adapter now retries the same idempotent update once when Monarch reports a retryable error or the network request times out. Caller cancellation and permanent errors return immediately. Each attempt records its own intent and completion in the API audit trail. + +**Verification:** +- The retry regression test failed before the fix and passes afterward. +- A retryable 502 results in two update attempts and four audit records (intent/completion for each attempt). +- A permanent Monarch validation error results in one update attempt. +- `go test ./...` passes. + +**Commit:** Included in the pull request for this fix. + ### 2026-07-30: Pending purchase categorization disappeared when transactions posted **Description:** diff --git a/go.mod b/go.mod index be57a65..b71eccd 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/eshaffer321/itemize -go 1.25.12 +go 1.25.13 require ( github.com/PuerkitoBio/goquery v1.12.0 diff --git a/internal/application/sync/consolidator.go b/internal/application/sync/consolidator.go index 19e005e..c71e640 100644 --- a/internal/application/sync/consolidator.go +++ b/internal/application/sync/consolidator.go @@ -3,11 +3,9 @@ package sync import ( "context" "encoding/json" - "errors" "fmt" "log/slog" "math" - "net" "strings" "time" @@ -232,18 +230,6 @@ func (c *Consolidator) updatePrimaryTransaction( return updated, nil } -func isRetryableMonarchError(ctx context.Context, err error) bool { - if ctx.Err() != nil || errors.Is(err, context.Canceled) { - return false - } - if monarch.IsRetryable(err) { - return true - } - - var networkErr net.Error - return errors.As(err, &networkErr) && networkErr.Timeout() -} - // deleteExtraTransactions removes the extra transactions after consolidation // Returns list of transaction IDs that failed to delete func (c *Consolidator) deleteExtraTransactions( diff --git a/internal/application/sync/monarch_retry.go b/internal/application/sync/monarch_retry.go new file mode 100644 index 0000000..8b92a3d --- /dev/null +++ b/internal/application/sync/monarch_retry.go @@ -0,0 +1,21 @@ +package sync + +import ( + "context" + "errors" + "net" + + "github.com/eshaffer321/monarch-go/v2/pkg/monarch" +) + +func isRetryableMonarchError(ctx context.Context, err error) bool { + if ctx.Err() != nil || errors.Is(err, context.Canceled) { + return false + } + if monarch.IsRetryable(err) { + return true + } + + var networkErr net.Error + return errors.As(err, &networkErr) && networkErr.Timeout() +} diff --git a/internal/application/sync/monarch_retry_test.go b/internal/application/sync/monarch_retry_test.go new file mode 100644 index 0000000..7da36fa --- /dev/null +++ b/internal/application/sync/monarch_retry_test.go @@ -0,0 +1,64 @@ +package sync + +import ( + "context" + "errors" + "net/url" + "testing" + + "github.com/eshaffer321/monarch-go/v2/pkg/monarch" + "github.com/stretchr/testify/assert" +) + +func TestIsRetryableMonarchError(t *testing.T) { + canceledCtx, cancel := context.WithCancel(context.Background()) + cancel() + + tests := []struct { + name string + ctx context.Context + err error + want bool + }{ + { + name: "caller canceled", + ctx: canceledCtx, + err: monarch.ErrServerError, + want: false, + }, + { + name: "canceled error", + ctx: context.Background(), + err: context.Canceled, + want: false, + }, + { + name: "Monarch server error", + ctx: context.Background(), + err: monarch.ErrServerError, + want: true, + }, + { + name: "network timeout", + ctx: context.Background(), + err: &url.Error{ + Op: "Post", + URL: "https://api.monarch.com/graphql", + Err: context.DeadlineExceeded, + }, + want: true, + }, + { + name: "permanent error", + ctx: context.Background(), + err: errors.New("invalid category"), + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, isRetryableMonarchError(tt.ctx, tt.err)) + }) + } +} diff --git a/internal/application/sync/process_order_test.go b/internal/application/sync/process_order_test.go index cdb29d9..c66b74c 100644 --- a/internal/application/sync/process_order_test.go +++ b/internal/application/sync/process_order_test.go @@ -204,6 +204,63 @@ func TestMonarchAdapter_LogAPICallRecordsIntentAndCompletion(t *testing.T) { assert.Contains(t, calls[1].ResponseJSON, "txn-intent") } +func TestMonarchAdapter_UpdateTransactionRetriesRetryableError(t *testing.T) { + store := storage.NewMockRepository() + transactions := &mockMonarchClient{ + updateErrors: []error{ + monarch.WrapError(monarch.ErrServerError, "SERVER_ERROR", "server error: 502 (Bad Gateway)"), + nil, + }, + } + adapter := &monarchAdapter{ + client: &monarch.Client{Transactions: transactions}, + storage: store, + logger: slog.New(slog.NewTextHandler(os.Stderr, nil)), + runID: 100, + } + + amount := -24.38 + notes := "Personal Care/Toiletries" + err := adapter.UpdateTransaction( + withAuditContext(context.Background(), "ORDER-RETRY", false), + "txn-retry", + &monarch.UpdateTransactionParams{Amount: &amount, Notes: ¬es}, + ) + + require.NoError(t, err) + assert.Equal(t, 2, transactions.updateCalled) + + calls, callErr := store.GetAPICallsByOrderID("ORDER-RETRY") + require.NoError(t, callErr) + require.Len(t, calls, 4) + assert.Equal(t, "intent", calls[0].Phase) + assert.Equal(t, "completed", calls[1].Phase) + assert.Contains(t, calls[1].Error, "502") + assert.Equal(t, "intent", calls[2].Phase) + assert.Equal(t, "completed", calls[3].Phase) + assert.Empty(t, calls[3].Error) +} + +func TestMonarchAdapter_UpdateTransactionDoesNotRetryPermanentError(t *testing.T) { + transactions := &mockMonarchClient{ + updateError: monarch.NewError("INVALID_REQUEST", "invalid category"), + } + adapter := &monarchAdapter{ + client: &monarch.Client{Transactions: transactions}, + } + + amount := -24.38 + notes := "Personal Care/Toiletries" + err := adapter.UpdateTransaction( + context.Background(), + "txn-permanent", + &monarch.UpdateTransactionParams{Amount: &amount, Notes: ¬es}, + ) + + require.Error(t, err) + assert.Equal(t, 1, transactions.updateCalled) +} + // ============================================================================= // Test: Generic Order Matching (Costco-like providers) // ============================================================================= diff --git a/internal/application/sync/types.go b/internal/application/sync/types.go index ca0e8bb..fdff192 100644 --- a/internal/application/sync/types.go +++ b/internal/application/sync/types.go @@ -192,11 +192,25 @@ type monarchAdapter struct { } func (a *monarchAdapter) UpdateTransaction(ctx context.Context, id string, params *monarch.UpdateTransactionParams) error { - a.logAPICallIntent(ctx, id, "Transactions.Update", params) - start := time.Now() - updated, err := a.client.Transactions.Update(ctx, id, params) - a.logAPICallCompletion(ctx, id, "Transactions.Update", updated, err, time.Since(start)) - return err + update := func() error { + a.logAPICallIntent(ctx, id, "Transactions.Update", params) + start := time.Now() + updated, err := a.client.Transactions.Update(ctx, id, params) + a.logAPICallCompletion(ctx, id, "Transactions.Update", updated, err, time.Since(start)) + return err + } + + err := update() + if err == nil || !isRetryableMonarchError(ctx, err) { + return err + } + if a.logger != nil { + a.logger.Warn("Transient transaction update failed; retrying", + "transaction_id", id, + "attempt", 1, + "error", err) + } + return update() } func (a *monarchAdapter) GetTransaction(ctx context.Context, id string) (*monarch.TransactionDetails, error) {