diff --git a/CLAUDE.md b/CLAUDE.md index 1ccbaa2..9986afa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -124,10 +124,23 @@ milliseconds. That is the checksum skip working, not a failure. **Money is `int64` minor units**, never a float. Per-account currency, no conversion, and totals are never summed across currencies. -**First matching rule wins**, so specific rules belong above general ones. -A rule setting both `match` and `type` requires both of them. -`config.AppendRule` therefore appends — never prepends — so saving from the -rule builder cannot shadow a rule the user wrote by hand. +**The most specific rule wins, not the topmost.** `rules.Engine` sorts the +rules once in `New` and matches in that order: most literal characters first, +then fewest `*`, then account-scoped over unscoped, with a *stable* sort so +equally specific rules keep file order and the earlier one still wins. That is +what lets `*NIKOLA*` carve an exception out of `*NIK*` from anywhere in the +file, and a catch-all `*` sit wherever it reads best. A rule setting both +`match` and `type` requires both of them, and both count towards its literals. + +The two orders must not be confused. `Engine.rules` stays in file order and +`Rules()`, `Usage` and `MatchIndex` all speak in file positions, because that +is what the rules screen numbers, what `config.DeleteRules` deletes by, and +what the user can point at in rules.toml; only `Engine.order` is sorted. +Anything new that reports a rule must report its file position too. + +`config.AppendRule` still appends rather than prepends, but that now only +settles ties: a saved rule cannot displace an equally specific one written by +hand, while a narrower one is meant to take precedence and does. **The builders are forms, so the global keymap must not apply there.** `Update` routes to `updateRules` / `updateTransfers` before `updateNormal` @@ -145,8 +158,8 @@ field navigation, and lets `ctrl+n` / `ctrl+p` through to the input for cycling. it afterwards or `ctrl+n` would offer candidates that no longer fit the value. **A rule's usage count is how many transactions it wins, not how many its glob -could match** — `Engine.Usage` counts by `MatchIndex`, so a rule shadowed by an -earlier one correctly reports zero. That is what makes the rules screen able to +could match** — `Engine.Usage` counts by `MatchIndex`, so a rule shadowed by a +more specific one correctly reports zero. That is what makes the rules screen able to find dead rules at all. **A rule's `note` is documentation that round-trips.** It is a TOML key rather diff --git a/README.md b/README.md index 2f245cc..bf9caa1 100644 --- a/README.md +++ b/README.md @@ -180,33 +180,40 @@ the folders on disk and the index; tags from every tag in use plus any named in which is what stops `groceries` acquiring a `grocery` twin. Leaving the account blank applies the rule everywhere; filling it in also -narrows the preview to that account. Rules are appended, so anything already in -`rules.toml` keeps precedence — the preview accounts for that automatically, -since it only ever lists transactions no existing rule has already tagged. Legs -of a matched transfer are left out too: they are already accounted for by the -definition that paired them, and the report leaves them out anyway. +narrows the preview to that account. The preview lists only transactions no +existing rule has already tagged, so it answers "what is still waiting for a +rule" rather than "what would this rule win". Those differ when the new rule is +narrower than an existing one: a `*LIDL SOFIA*` written while `*LIDL*` is +already tagging those rows previews as nothing and still takes them on save, +because the more specific rule wins. Legs of a matched transfer are left out +too: they are already accounted for by the definition that paired them, and the +report leaves them out anyway. ### Rules (`5`) -Every rule in file order, with the number of transactions it actually claims. -Rules that claim none are marked `✗`. +Every rule in file order — the order they are written in, not the order they +are tried in — with the number of transactions it actually claims. Rules that +claim none are marked `✗`. ``` money · rules · 5 rules · 2 match nothing # Pattern Account Tag Txns Note -1 *LIDL* (all) groceries 3 the weekly shop -2 ✗ *LIDL SOFIA* (all) shadowed 0 +1 *LIDL SOFIA* (all) groceries 3 the weekly shop +2 ✗ *LIDL* (all) shadowed 0 3 ✗ *OLD BANK NAME* (all) dead 0 closed in 2025 4 *ZARA* (all) clothes 1 5 *КАУФЛАНД* checking groceries 1 4412 is the branch ``` The count is how many transactions the rule *wins*, not how many its glob could -match. Rule 2 above matches `LIDL SOFIA 4412` perfectly well, but rule 1 claims -it first, so rule 2 is dead weight — which is the point of the screen. A rule -can therefore reach zero either by matching nothing or by being shadowed, and -both are worth deleting. +match. Rule 2 above matches `LIDL SOFIA 4412` perfectly well, but rule 1 spells +out more of it and so claims it first; with nothing else here that says `LIDL`, +rule 2 is dead weight — which is the point of the screen. A rule can therefore +reach zero either by matching nothing or by being shadowed, and both are worth +deleting. Note that shadowing has nothing to do with the numbering: rule 2 would +be just as dead written above rule 1, because precedence is decided by how +specific a rule is and not by where it sits. `d` deletes the selected rule, `p` deletes every rule marked `✗` at once, and both ask for a `y` first. `r` recounts against what is currently in the index, @@ -317,9 +324,24 @@ it. `r` re-pairs against what is currently in the index. ## rules.toml -Rules are evaluated in file order and the **first match wins**, so put specific -rules above general ones. Patterns are globs (`*` and `?`) matched -case-insensitively, with whitespace collapsed. +**The most specific rule wins**, so `*NIKOLA*` claims what it names even with a +broad `*NIK*` sitting above it, and a catch-all can be written anywhere without +swallowing the file. Patterns are globs (`*` and `?`) matched case-insensitively, +with whitespace collapsed. + +Specificity is measured on what a rule spells out, in this order: + +1. how many literal characters its patterns pin down — `*LIDL SOFIA*` (10) + beats `*LIDL*` (4), and a bare `*` (0) is tried last of all. A rule setting + both `match` and `type` has to satisfy both, so both count. +2. how few `*` it uses, the only wildcard that swallows a run of any length: + `LIDL` fixes both ends where `*LIDL*` does not, so it goes first. (`?` is + not counted either way: it fixes a length, not a character.) +3. whether it names an `account`, which is a narrowing the unscoped rule with + the same patterns does not have. + +Rules that tie on all three fall back to file order, and the earlier one wins — +which is what keeps appending a rule from disturbing one already written. A rule matches on `match` (the description) and `type` (the bank's own classification). Setting both is an "and": both must match. diff --git a/internal/config/config.go b/internal/config/config.go index 9e9a418..4322016 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -23,8 +23,9 @@ const ( IndexFile = "index.db" ) -// Rule is one entry in rules.toml. Rules are evaluated in file order and the -// first one whose Match (and optional Account) matches wins. +// Rule is one entry in rules.toml. The most specific rule that matches wins, +// with file order breaking ties between equally specific ones; rules.Engine +// owns that ordering. type Rule struct { Match string `toml:"match"` Tag string `toml:"tag"` @@ -126,8 +127,10 @@ func checkTransfer(t Transfer) error { } // AppendRule adds a rule to the end of rules.toml, creating the file if it is -// not there yet. Appending rather than inserting means an existing rule always -// keeps precedence, since the first match wins. +// not there yet. Position no longer decides precedence — the most specific rule +// wins, see rules.Engine — but it still breaks ties, so appending rather than +// inserting keeps a saved rule from displacing an equally specific one the user +// wrote by hand. // // The file is rewritten through a temporary file so a failure part-way cannot // leave the user with a truncated config. diff --git a/internal/rules/rules.go b/internal/rules/rules.go index a20bf99..1759b1c 100644 --- a/internal/rules/rules.go +++ b/internal/rules/rules.go @@ -1,26 +1,94 @@ -// Package rules applies the ordered glob rules from rules.toml to -// transactions, deciding their tag. It is the only thing that decides a tag, -// so rule_tag is derived state and Retag can rewrite it wholesale at any time. +// Package rules applies the glob rules from rules.toml to transactions, +// deciding their tag. The most specific rule that fits wins, so a general rule +// and the narrower one carving an exception out of it can be written in either +// order. It is the only thing that decides a tag, so rule_tag is derived state +// and Retag can rewrite it wholesale at any time. package rules import ( + "sort" + "git.petrovv.com/nikola/money/internal/config" "git.petrovv.com/nikola/money/internal/glob" "git.petrovv.com/nikola/money/internal/model" "git.petrovv.com/nikola/money/internal/store" ) -// Engine evaluates rules in file order; the first match wins. +// Engine evaluates rules most specific first; the first match wins. type Engine struct { + // rules is file order, which is what Rules and Usage report and what the + // rules screen numbers and deletes by. Evaluation does not use it. rules []config.Rule + // order indexes into rules, most specific first. Specificity decides + // precedence rather than position, so "*NIKOLA*" claims what it names + // wherever it sits relative to the "*NIK*" that would otherwise swallow it. + order []int } -// New builds an engine from the parsed rules file. +// New builds an engine from the parsed rules file, working out once which rule +// is tried before which. func New(r *config.Rules) *Engine { - return &Engine{rules: r.Rule} + e := &Engine{rules: r.Rule, order: make([]int, len(r.Rule))} + for i := range e.order { + e.order[i] = i + } + // Stable, so rules of equal specificity keep file order between them and + // the earlier one still wins. + sort.SliceStable(e.order, func(a, b int) bool { + return moreSpecific(e.rules[e.order[a]], e.rules[e.order[b]]) + }) + return e } -// Rules returns the ordered rules, as loaded from rules.toml. +// moreSpecific reports whether a is tried before b. +// +// The literal characters a rule spells out are the evidence: they are what it +// commits to, and what it hands to a '*' is what it gives up. So "*NIKOLA*" +// beats "*NIK*", and a bare "*" sits last of all — the catch-all can be written +// anywhere in the file and still catch only what nothing else wanted. +// +// Two rules spelling out the same amount are separated by what else they pin +// down: fewer '*' first, since "NIKOLA" also fixes both ends where "*NIKOLA*" +// does not, and then an account-scoped rule over one that applies everywhere. +// Rules that tie on all three are left to file order by the stable sort. +func moreSpecific(a, b config.Rule) bool { + if x, y := literals(a), literals(b); x != y { + return x > y + } + if x, y := stars(a), stars(b); x != y { + return x < y + } + return a.Account != "" && b.Account == "" +} + +// literals counts the characters a rule pins down exactly, across every pattern +// it sets: a rule with both match and type has to satisfy both, so both count. +// '?' is not one of them — it fixes a length, not a character. +func literals(r config.Rule) int { + n := 0 + for _, c := range r.Match + r.Type { + if c != '*' && c != '?' { + n++ + } + } + return n +} + +// stars counts the unbounded wildcards, the only ones that let a pattern match +// a run of any length. +func stars(r config.Rule) int { + n := 0 + for _, c := range r.Match + r.Type { + if c == '*' { + n++ + } + } + return n +} + +// Rules returns the rules in file order, as loaded from rules.toml. That is the +// order the rules screen shows and deletes by; it is no longer the order they +// are tried in. func (e *Engine) Rules() []config.Rule { return e.rules } // Match returns the first rule matching a transaction on the given account, or @@ -32,19 +100,21 @@ func (e *Engine) Match(accountSlug string, t model.Transaction) *config.Rule { return nil } -// MatchIndex returns the position of the first rule matching a transaction, or +// MatchIndex returns the file position of the rule claiming a transaction, or // -1 if none does. Every pattern a rule sets must match: a rule with both // match and type is an "and", not an "or". // -// The position matters as well as the rule: because the first match wins, a -// rule that is fully shadowed by an earlier one never applies to anything, and -// only the index reveals that. +// Rules are tried most specific first, so the answer is the narrowest rule that +// fits and not merely the topmost. The returned index is still the position in +// rules.toml, because that is what the caller can point the user at — and it is +// what reveals a rule that is fully shadowed by a more specific one and so +// never applies to anything. func (e *Engine) MatchIndex(accountSlug string, t model.Transaction) int { var ( description = model.NormalizeDescription(t.Description) kind = model.NormalizeDescription(t.Type) ) - for i := range e.rules { + for _, i := range e.order { r := &e.rules[i] if r.Account != "" && r.Account != accountSlug { continue @@ -60,9 +130,9 @@ func (e *Engine) MatchIndex(accountSlug string, t model.Transaction) int { return -1 } -// Usage counts how many transactions each rule actually claims. A rule with a -// count of zero is dead: either nothing matches it, or an earlier rule takes -// everything it would have caught. +// Usage counts how many transactions each rule actually claims, indexed by file +// position. A rule with a count of zero is dead: either nothing matches it, or +// a more specific rule takes everything it would have caught. func (e *Engine) Usage(txns []model.Transaction) []int { counts := make([]int, len(e.rules)) for _, t := range txns { diff --git a/internal/rules/rules_test.go b/internal/rules/rules_test.go index 7f4fb1c..d0e3408 100644 --- a/internal/rules/rules_test.go +++ b/internal/rules/rules_test.go @@ -108,35 +108,72 @@ func TestRetagRewritesEveryTag(t *testing.T) { } } -func TestFirstMatchWins(t *testing.T) { - engine := New(&config.Rules{Rule: []config.Rule{ - {Match: "*LIDL EXPRESS*", Tag: "snacks"}, - {Match: "*LIDL*", Tag: "groceries"}, - }}) - if tag := engine.Apply("checking", "CARD LIDL EXPRESS 12"); tag != "snacks" { - t.Errorf("tag = %q, want snacks (earlier rule must win)", tag) +// The narrower rule claims what it names wherever it sits in the file: the +// catch-all above it must not swallow the exception written below. +func TestMoreSpecificRuleWins(t *testing.T) { + both := [][]config.Rule{ + {{Match: "NIK*", Tag: "misc"}, {Match: "NIKOLA*", Tag: "family"}}, + {{Match: "NIKOLA*", Tag: "family"}, {Match: "NIK*", Tag: "misc"}}, } - if tag := engine.Apply("checking", "CARD LIDL 12"); tag != "groceries" { - t.Errorf("tag = %q, want groceries", tag) + for _, rs := range both { + engine := New(&config.Rules{Rule: rs}) + if tag := engine.Apply("checking", "NIKOLA PETROV"); tag != "family" { + t.Errorf("%q first: tag = %q, want family", rs[0].Match, tag) + } + if tag := engine.Apply("checking", "NIKI TODOROV"); tag != "misc" { + t.Errorf("%q first: tag = %q, want misc", rs[0].Match, tag) + } + } +} + +// Specificity is measured on what the pattern spells out, so the wildcards +// around a literal do not buy it precedence, and a bare "*" is always last. +func TestSpecificityOrder(t *testing.T) { + engine := New(&config.Rules{Rule: []config.Rule{ + {Match: "*", Tag: "other"}, + {Match: "*LIDL*", Tag: "groceries"}, + {Match: "*LIDL EXPRESS*", Tag: "snacks"}, + }}) + for _, c := range []struct{ desc, want string }{ + {"CARD LIDL EXPRESS 12", "snacks"}, + {"CARD LIDL 12", "groceries"}, + {"SOMETHING ELSE", "other"}, + } { + if tag := engine.Apply("checking", c.desc); tag != c.want { + t.Errorf("%q: tag = %q, want %q", c.desc, tag, c.want) + } + } +} + +// Two rules that are equally specific are still decided by the file: the +// earlier one wins, which is what makes appending a rule safe. +func TestEqualSpecificityKeepsFileOrder(t *testing.T) { + engine := New(&config.Rules{Rule: []config.Rule{ + {Match: "*ACME*", Tag: "first"}, + {Match: "*ACME*", Tag: "second"}, + }}) + if tag := engine.Apply("checking", "ACME LTD"); tag != "first" { + t.Errorf("tag = %q, want first", tag) } if tag := engine.Apply("checking", "SOMETHING ELSE"); tag != "" { t.Errorf("tag = %q, want empty for an unmatched description", tag) } } -// A rule can be dead two ways: nothing matches it, or an earlier rule already -// claimed everything it would have caught. Usage must report both as zero. -func TestUsageCountsFirstMatchOnly(t *testing.T) { +// A rule can be dead two ways: nothing matches it, or a rule that beats it +// already claimed everything it would have caught. Usage must report both as +// zero, indexed by file position however the rules are ordered for matching. +func TestUsageCountsWinnersOnly(t *testing.T) { engine := New(&config.Rules{Rule: []config.Rule{ - {Match: "*LIDL*", Tag: "groceries"}, // claims both LIDL rows - {Match: "*LIDL SOFIA*", Tag: "shadowed"}, // fully shadowed by the above + {Match: "*LIDL SOFIA*", Tag: "groceries"}, // the narrower of the two + {Match: "*LIDL*", Tag: "shadowed"}, // every LIDL row here is Sofia {Match: "*NOTHING MATCHES ME*", Tag: "no"}, // matches nothing at all {Match: "*PAYROLL*", Tag: "salary"}, // claims one row }}) txns := []model.Transaction{ {AccountSlug: "checking", Description: "LIDL SOFIA 4412"}, - {AccountSlug: "checking", Description: "LIDL VARNA 9911"}, + {AccountSlug: "checking", Description: "LIDL SOFIA 9911"}, {AccountSlug: "checking", Description: "ACME PAYROLL"}, {AccountSlug: "checking", Description: "UNMATCHED SHOP"}, } @@ -153,27 +190,31 @@ func TestUsageCountsFirstMatchOnly(t *testing.T) { } } +// The index is the rule's position in rules.toml, not its position in the order +// it was tried in — that is the number the rules screen shows and deletes by. func TestMatchIndex(t *testing.T) { engine := New(&config.Rules{Rule: []config.Rule{ - {Match: "*LIDL EXPRESS*", Tag: "snacks"}, {Match: "*LIDL*", Tag: "groceries"}, + {Match: "*LIDL EXPRESS*", Tag: "snacks"}, }}) - if got := engine.MatchIndex("checking", model.Transaction{Description: "LIDL EXPRESS 1"}); got != 0 { - t.Errorf("index = %d, want 0", got) - } - if got := engine.MatchIndex("checking", model.Transaction{Description: "LIDL 1"}); got != 1 { + if got := engine.MatchIndex("checking", model.Transaction{Description: "LIDL EXPRESS 1"}); got != 1 { t.Errorf("index = %d, want 1", got) } + if got := engine.MatchIndex("checking", model.Transaction{Description: "LIDL 1"}); got != 0 { + t.Errorf("index = %d, want 0", got) + } if got := engine.MatchIndex("checking", model.Transaction{Description: "OTHER"}); got != -1 { t.Errorf("index = %d, want -1 for no match", got) } } +// Naming an account is itself a narrowing, so the scoped rule beats the +// identical unscoped one even when the unscoped one is written first. func TestAccountScopedRule(t *testing.T) { engine := New(&config.Rules{Rule: []config.Rule{ - {Match: "*TRANSFER*", Tag: "transfer", Account: "savings"}, {Match: "*TRANSFER*", Tag: "misc"}, + {Match: "*TRANSFER*", Tag: "transfer", Account: "savings"}, }}) if tag := engine.Apply("savings", "TRANSFER FROM CHECKING"); tag != "transfer" { diff --git a/internal/tui/tui.go b/internal/tui/tui.go index 8be40b2..e8b19d5 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -664,8 +664,9 @@ func acceptCompletion(in *textinput.Model) bool { // reloadRuleList counts, for every rule, how many transactions it actually // claims. A rule can match nothing because no description fits it, or because -// an earlier rule already took everything it would have caught; both show up -// here as a zero. +// a more specific rule already took everything it would have caught; both show +// up here as a zero. The rows stay in file order — that is the number the user +// can find in rules.toml, and precedence is not read off it any more. func (m *Model) reloadRuleList() error { txns, err := m.db.Transactions(store.Filter{}) if err != nil { diff --git a/internal/tui/tui_test.go b/internal/tui/tui_test.go index 2b1caee..4e8f079 100644 --- a/internal/tui/tui_test.go +++ b/internal/tui/tui_test.go @@ -807,8 +807,10 @@ func newRuleListModel(t *testing.T) (*Model, string) { match = "*LIDL*" tag = "groceries" +# Dead: the rule above is just as specific and gets there first, so this one +# never claims anything. A narrower pattern would have won instead. [[rule]] -match = "*LIDL SOFIA*" +match = "*LIDL*" tag = "shadowed" [[rule]]