From 2373790be3c8c4fe931339ea56d17d0d348794a4 Mon Sep 17 00:00:00 2001 From: Lars Nolden Date: Sat, 12 Sep 2026 18:35:38 +0200 Subject: [PATCH] Let reviews see low-confidence suggestions and rebase preview applies A low-confidence answer was discarded inside Classify, so Analyse showed the row as unchanged instead of a reviewable suggestion; the fallback decision moves to the import path, which keeps the merchant link and the recorded confidence. ApplyPreview now rebases onto the current journal: unrelated commits during a minutes-long paced run no longer invalidate the review, only an edit to a selected transaction itself conflicts, and applied changes are pruned so the rest stay appliable. --- internal/app/app_test.go | 76 ++++++++++++++++++++++++++ internal/app/import.go | 17 ++++++ internal/app/reclassify.go | 60 ++++++++++++++++---- internal/classification/client.go | 3 - internal/classification/client_test.go | 8 ++- web/src/Classification.tsx | 24 ++++---- 6 files changed, 160 insertions(+), 28 deletions(-) diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 7281bcc..f1c9ecd 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -369,6 +369,82 @@ func TestStalePreviewCannotOverwriteManualCorrection(t *testing.T) { t.Fatal("stale apply partially changed records") } } + +// A preview run is minutes long by design (paced provider calls), so a +// scheduled sync, an import, or an earlier partial apply committing in the +// meantime must not invalidate the review: only an edit to a selected +// transaction itself conflicts. +func TestApplyPreviewSurvivesUnrelatedCommitsAndPartialApplies(t *testing.T) { + ctx := context.Background() + a, s := testApp(t) + s = seed(t, a, s) + mockClassifier(t, a) + p, err := runPreview(t, a, PreviewRequest{Revision: s.Revision, From: "2026-09-01", To: "2026-09-30", Model: "test/model", Fields: Fields{Category: true}}) + if err != nil { + t.Fatal(err) + } + if len(p.Changes) != 2 { + t.Fatalf("expected two proposed changes: %+v", p) + } + // An unrelated registry edit moves the journal revision after the preview. + if _, err = a.Mutate(ctx, s.Revision, func(d *domain.Dataset) error { + d.Tags = append(d.Tags, domain.Tag{ID: "travel", Name: "travel"}) + return nil + }); err != nil { + t.Fatal(err) + } + first, err := a.ApplyPreview(ctx, p.ID, p.Revision, []string{p.Changes[0].ID}) + if err != nil { + t.Fatalf("unrelated commit invalidated the preview: %v", err) + } + // The partial apply moved the revision again; the remaining proposal must + // still apply without another paced provider run. + second, err := a.ApplyPreview(ctx, p.ID, p.Revision, []string{p.Changes[1].ID}) + if err != nil { + t.Fatalf("partial apply consumed the remaining proposals: %v", err) + } + if second.Revision == first.Revision { + t.Fatal("second apply committed nothing") + } + for _, tx := range second.Data.Transactions { + if tx.Enrichment.CategoryID != "groceries" { + t.Fatalf("applied categories lost: %+v", tx.Enrichment) + } + } + // Both changes are consumed now; re-applying must fail, not double-write. + if _, err = a.ApplyPreview(ctx, p.ID, p.Revision, []string{p.Changes[0].ID}); err == nil { + t.Fatal("consumed change applied twice") + } +} + +// Imports auto-apply only what the model is sure about: a low-confidence +// category lands on the editable fallback while the merchant link and the +// recorded confidence survive for review in Analyse. +func TestImportNeverAutoAppliesLowConfidenceCategory(t *testing.T) { + a, s := testApp(t) + provider := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + content := `{"merchant_id":null,"new_merchant":"REWE","category_id":"groceries","tag_ids":[],"confidence":"low"}` + json.NewEncoder(w).Encode(map[string]any{"choices": []any{map[string]any{ + "finish_reason": "stop", + "message": map[string]any{"content": content}, + }}}) + })) + defer provider.Close() + a.classifier = classification.Client{APIKey: "test", Model: "test/model", BaseURL: provider.URL} + s = seed(t, a, s) + if len(s.Data.Transactions) != 2 { + t.Fatalf("import lost transactions: %d", len(s.Data.Transactions)) + } + for _, tx := range s.Data.Transactions { + e := tx.Enrichment + if e.CategoryID != domain.ExpenseFallback { + t.Fatalf("low-confidence category was auto-applied: %+v", e) + } + if e.MerchantID == "" || e.Classification.Confidence != "low" || e.Classification.Source != "openrouter" { + t.Fatalf("merchant link or provenance lost: %+v", e) + } + } +} func TestTaxonomyProposalApprovalMintsOnlyApprovedEntries(t *testing.T) { a, s := testApp(t) s = seed(t, a, s) diff --git a/internal/app/import.go b/internal/app/import.go index daf0909..6ba7854 100644 --- a/internal/app/import.go +++ b/internal/app/import.go @@ -82,6 +82,12 @@ func (a *App) importFacts(ctx context.Context, s State, facts []domain.Facts, in p, e := classification.Rules(t.Facts, s.Data) if a.settings.ClassifyOnImport { p, e = a.classifier.Classify(ctx, t.Facts, s.Data, false) + // A low-confidence category is never auto-applied on import: the + // merchant link and provenance stay, and Analyse shows the model's + // suggestion for review instead. + if e == nil && p.Enrichment.Classification.Confidence == "low" { + p.Enrichment.CategoryID = domain.Fallback(t.Facts).CategoryID + } } if e == nil { e = addProposal(&s.Data, p, t.Facts) @@ -927,6 +933,13 @@ func syncBackoff(now time.Time, ops operational) time.Duration { func (a *App) RunScheduler(ctx context.Context) { timer := time.NewTimer(time.Minute) defer timer.Stop() + // Prices keep their own clock: they come from a different provider, they are + // wanted even when no bank is connected, and a sync backoff must not delay + // them. The first run is shortly after start, so a fresh install or a + // restart does not leave a day's holdings unvalued waiting for the tick; + // after that it is daily, which is as often as a close changes. + prices := time.NewTimer(quoteStartup) + defer prices.Stop() for { force := false select { @@ -934,6 +947,10 @@ func (a *App) RunScheduler(ctx context.Context) { return case <-a.syncRequested: force = true + case <-prices.C: + a.RefreshQuotes(ctx) + prices.Reset(quoteInterval) + continue case <-timer.C: } a.mu.Lock() diff --git a/internal/app/reclassify.go b/internal/app/reclassify.go index b28f2c3..8e14504 100644 --- a/internal/app/reclassify.go +++ b/internal/app/reclassify.go @@ -276,6 +276,21 @@ func classifyRange(ctx context.Context, client *classification.Client, s State, p.NewMerchants = append([]domain.Merchant{}, s.Data.Merchants[baseMerchants:]...) return p, nil } +// enrichmentEqual compares enrichment semantically: tag order is not a change. +func enrichmentEqual(a, b domain.Enrichment) bool { + a.TagIDs = slices.Clone(a.TagIDs) + b.TagIDs = slices.Clone(b.TagIDs) + slices.Sort(a.TagIDs) + slices.Sort(b.TagIDs) + return reflect.DeepEqual(a, b) +} + +// ApplyPreview rebases the selected proposals onto the current journal. A +// preview run is minutes long by design, so unrelated commits (a scheduled +// sync, an import, an earlier partial apply of this same preview) must not +// invalidate the review; only a selected transaction whose own enrichment +// changed since the preview snapshot conflicts. Applied changes are pruned so +// the remaining proposals stay appliable without another paced provider run. func (a *App) ApplyPreview(ctx context.Context, id, rev string, ids []string) (State, error) { a.mu.Lock() defer a.mu.Unlock() @@ -290,12 +305,9 @@ func (a *App) ApplyPreview(ctx context.Context, id, rev string, ids []string) (S if err != nil { return State{}, err } - if s.Revision != rev { - return State{}, errors.New("revision conflict: data changed after preview; analyse again") - } - changes := map[string]domain.Enrichment{} + changes := map[string]Change{} for _, c := range p.Changes { - changes[c.ID] = c.After + changes[c.ID] = c } selected := map[string]bool{} for _, id := range ids { @@ -307,16 +319,29 @@ func (a *App) ApplyPreview(ctx context.Context, id, rev string, ids []string) (S if len(selected) == 0 { return State{}, errors.New("select at least one change") } + applied := 0 needed := map[string]bool{} for i, t := range s.Data.Transactions { if !selected[t.Facts.ID] { continue } - s.Data.Transactions[i].Enrichment = changes[t.Facts.ID] - needed[changes[t.Facts.ID].MerchantID] = true + c := changes[t.Facts.ID] + if !enrichmentEqual(t.Enrichment, c.Before) { + return State{}, errors.New("revision conflict: a selected transaction changed after the preview; analyse it again") + } + s.Data.Transactions[i].Enrichment = c.After + needed[c.After.MerchantID] = true + applied++ + } + if applied != len(selected) { + return State{}, errors.New("revision conflict: a selected transaction no longer exists; analyse again") + } + existing := map[string]bool{} + for _, m := range s.Data.Merchants { + existing[m.ID] = true } for _, m := range p.NewMerchants { - if needed[m.ID] { + if needed[m.ID] && !existing[m.ID] { s.Data.Merchants = append(s.Data.Merchants, m) } } @@ -327,14 +352,25 @@ func (a *App) ApplyPreview(ctx context.Context, id, rev string, ids []string) (S LearnAlias(&s.Data, t.Facts, t.Enrichment.MerchantID) } } - state, err := a.commit(ctx, rev, s.Data) + state, err := a.commit(ctx, s.Revision, s.Data) if err != nil { return State{}, err } - delete(a.previews, id) - if job := a.previewRun; job != nil && job.status.ID == id { - a.previewRun = nil + kept := make([]Change, 0, len(p.Changes)-applied) + for _, c := range p.Changes { + if !selected[c.ID] { + kept = append(kept, c) + } } + if len(kept) == 0 { + delete(a.previews, id) + if job := a.previewRun; job != nil && job.status.ID == id { + a.previewRun = nil + } + return state, nil + } + p.Changes = kept + a.previews[id] = p return state, nil } diff --git a/internal/classification/client.go b/internal/classification/client.go index 5cd9105..34a8a0d 100644 --- a/internal/classification/client.go +++ b/internal/classification/client.go @@ -258,9 +258,6 @@ func (c *Client) Classify(ctx context.Context, facts domain.Facts, data domain.D } } e.Classification = domain.Provenance{Source: "openrouter", Model: model, Confidence: answer.Confidence, Timestamp: time.Now().UTC().Format(time.RFC3339)} - if answer.Confidence == "low" { - e.CategoryID = domain.Fallback(facts).CategoryID - } validationData := data if proposed != nil { validationData.Merchants = append(append([]domain.Merchant{}, data.Merchants...), *proposed) diff --git a/internal/classification/client_test.go b/internal/classification/client_test.go index 372d5dd..5374bb1 100644 --- a/internal/classification/client_test.go +++ b/internal/classification/client_test.go @@ -438,7 +438,7 @@ func TestConfiguredPrivateNamesAndIdentifiersRedactWithoutRemovingPayee(t *testi } } -func TestLowConfidenceKeepsMerchantAndTagsButUsesFallback(t *testing.T) { +func TestLowConfidenceKeepsProposalAndRecordsConfidence(t *testing.T) { f, d := fixture() c := mockClient(t, func(w http.ResponseWriter, r *http.Request) { reply(w, `{"merchant_id":"mer_coffee","new_merchant":null,"category_id":"cat_food","tag_ids":["tag_daily"],"confidence":"low"}`) @@ -447,11 +447,13 @@ func TestLowConfidenceKeepsMerchantAndTagsButUsesFallback(t *testing.T) { if err != nil { t.Fatal(err) } - if p.Enrichment.CategoryID != domain.ExpenseFallback || + // Review flows need the model's suggestion; discarding it is the import + // path's decision, not the client's. + if p.Enrichment.CategoryID != "cat_food" || p.Enrichment.MerchantID != "mer_coffee" || !reflect.DeepEqual(p.Enrichment.TagIDs, []string{"tag_daily"}) || p.Enrichment.Classification.Confidence != "low" { - t.Fatalf("low-confidence proposal was not preserved safely: %+v", p) + t.Fatalf("low-confidence proposal was not preserved: %+v", p) } } diff --git a/web/src/Classification.tsx b/web/src/Classification.tsx index aad48e9..f58a02e 100644 --- a/web/src/Classification.tsx +++ b/web/src/Classification.tsx @@ -359,9 +359,10 @@ export function Classification({ {preview.revision !== state.revision && ( -
- Your journal changed since this preview. Cancel it and generate a - fresh preview before applying. +
+ Your journal changed since this preview. Selected changes still + apply as long as their transactions were not edited in the + meantime.
)}
@@ -449,11 +450,7 @@ export function Classification({