diff --git a/FIELDS.md b/FIELDS.md index e19c7479..ee7a416f 100644 --- a/FIELDS.md +++ b/FIELDS.md @@ -67,7 +67,7 @@ Nested inside `Transaction.subtransactions` for split transactions. ### Category **All fields**: `id`, `category_group_id`, `category_group_name`, `name`, `hidden`, -`original_category_group_id`, `note`, `budgeted`, `activity`, `balance`, `goal_type`, +`internal`, `original_category_group_id`, `note`, `budgeted`, `activity`, `balance`, `goal_type`, `goal_needs_whole_amount`, `goal_day`, `goal_cadence`, `goal_cadence_frequency`, `goal_creation_month`, `goal_target`, `goal_target_month`, `goal_target_date`, `goal_percentage_complete`, `goal_months_to_budget`, `goal_under_funded`, @@ -89,10 +89,17 @@ for budgeting analysis. ### CategoryGroup -**All fields**: `id`, `name`, `hidden`, `deleted`, `categories` +**All fields**: `id`, `name`, `hidden`, `internal`, `deleted`, `categories` **Default `exclude_fields`**: `["deleted"]` +`internal` marks a group YNAB maintains itself (`Internal Master Category`, +`Credit Card Payments`, `Hidden Categories`) rather than one the user created. +It is kept in responses by default because it is what analytics exclude on. Note +it does **not** inherit: a Credit Card Payment category is `internal: false` +inside an `internal: true` group, so the group's flag is the meaningful one. +`null` means the API did not return the flag. + ### MonthSummary / MonthDetail **MonthSummary fields**: `month`, `note`, `income`, `budgeted`, `activity`, diff --git a/src/cache/service.py b/src/cache/service.py index 61b7a04a..af574f7c 100644 --- a/src/cache/service.py +++ b/src/cache/service.py @@ -176,7 +176,7 @@ async def api_call(): # Store group without nested categories to avoid duplication flat_groups.append(CategoryGroup( id=group.id, name=group.name, hidden=group.hidden, - deleted=group.deleted, categories=[], + internal=group.internal, deleted=group.deleted, categories=[], )) await self.delta.upsert_entities(plan_id, ENTITY_CATEGORY_GROUP, flat_groups) diff --git a/src/models/category.py b/src/models/category.py index 57ba5736..2ffd7401 100644 --- a/src/models/category.py +++ b/src/models/category.py @@ -23,6 +23,12 @@ class Category(YNABBaseModel): category_group_name: str | None = Field(default=None) name: str = Field() hidden: bool = Field(default=False) + # Required on both models in YNAB spec 1.86.0. Nullable here so an absent + # flag is distinguishable from a false one: absent must degrade to the legacy + # group-name check, false must not. Note the flag does NOT inherit — Credit + # Card Payment categories are internal=false inside an internal=true group, + # so the *group's* flag is the one that means "not user spending". + internal: bool | None = Field(default=None) original_category_group_id: str | None = Field(default=None) note: str | None = Field(default=None) budgeted: Milliunit = Field(default=0) @@ -50,5 +56,6 @@ class CategoryGroup(YNABBaseModel): id: str = Field() name: str = Field() hidden: bool = Field(default=False) + internal: bool | None = Field(default=None) deleted: bool = Field(default=False) categories: list[Category] = Field(default=[]) diff --git a/src/server/analytics.py b/src/server/analytics.py index 2a680386..05e06372 100644 --- a/src/server/analytics.py +++ b/src/server/analytics.py @@ -3,8 +3,17 @@ from src.models.common import milliunits_to_dollars from src.server import _shared +from src.server.internal_categories import InternalCategoryResolver -MONEY_FLOW_EXCLUDE_GROUPS = {"Internal Master Category", "Credit Card Payments"} + +async def _internal_resolver(plan_id: str) -> InternalCategoryResolver: + """Resolve the category -> group.internal map for a plan. + + One extra delta-synced (cached) call to GET /categories; the month payload + does not carry the group's flag. + """ + groups = await _shared.cache.get_categories(plan_id) + return InternalCategoryResolver(groups) @_shared.mcp.tool() @@ -24,14 +33,16 @@ async def get_money_flow(plan_id: str, month: str = "current") -> str: # Fetch month detail (has categories with activity and income total) month_detail = await _shared.cache.get_month(month, plan_id) + internal = await _internal_resolver(plan_id) # Group categories by category_group_name and sum activity group_activity: dict[str, int] = {} for cat in month_detail.categories: - group_name = cat.category_group_name or "Uncategorized" - if group_name in MONEY_FLOW_EXCLUDE_GROUPS: + if internal.is_internal(cat): continue + group_name = cat.category_group_name or "Uncategorized" group_activity[group_name] = group_activity.get(group_name, 0) + cat.activity + internal.log_fallbacks() # Filter out groups with zero activity group_activity = {name: activity for name, activity in group_activity.items() if activity != 0} @@ -75,14 +86,15 @@ async def get_spending_by_category(plan_id: str, month: str = "current") -> str: month = today.replace(day=1).strftime("%Y-%m-%d") month_detail = await _shared.cache.get_month(month, plan_id) + internal = await _internal_resolver(plan_id) # Collect categories with non-zero activity, excluding internal groups categories = [] total_spent_mu = 0 for cat in month_detail.categories: - group_name = cat.category_group_name or "Uncategorized" - if group_name in MONEY_FLOW_EXCLUDE_GROUPS: + if internal.is_internal(cat): continue + group_name = cat.category_group_name or "Uncategorized" if cat.activity == 0: continue spent_mu = abs(cat.activity) @@ -95,6 +107,8 @@ async def get_spending_by_category(plan_id: str, month: str = "current") -> str: "balance_mu": cat.balance, }) + internal.log_fallbacks() + # Sort by spending (highest first) and compute percentages categories.sort(key=lambda c: c["spent_mu"], reverse=True) result_cats = [] diff --git a/src/server/internal_categories.py b/src/server/internal_categories.py new file mode 100644 index 00000000..5a319b8a --- /dev/null +++ b/src/server/internal_categories.py @@ -0,0 +1,101 @@ +"""Which categories are YNAB's own bookkeeping rather than user spending. + +Analytics must exclude YNAB-internal categories, or credit-funded spending is +counted twice: a Credit Card Payment category's `activity` is the payment +movement for spending already counted in the real category. + +**The predicate is the category's *group's* `internal` flag.** Measured against a +live budget: `Internal Master Category`, `Credit Card Payments`, and +`Hidden Categories` all carry `group.internal = true`, while every Credit Card +Payment *category* carries `internal = false`. The flag does not inherit. Using +the category-level flag would readmit every payment category; matching group +names by string is locale-dependent, breaks on rename, and can be spoofed by a +user-created group of the same name. + +Nothing here knows how many such categories exist in any particular budget. +""" + +import logging + +from src.models import Category, CategoryGroup + +logger = logging.getLogger(__name__) + +# Fallback only, for a category whose group flag cannot be resolved: a historical +# month can name a category absent from the current category list, and a future +# API could stop returning `internal`. +LEGACY_INTERNAL_GROUP_NAMES = frozenset( + {"Internal Master Category", "Credit Card Payments"} +) + + +class InternalCategoryResolver: + """Decides, per category, whether it belongs to a YNAB-internal group. + + `group.internal` comes from `GET /plans/{id}/categories`; month analytics read + `GET /plans/{id}/months/{month}`, which carries the category-level flag but + not the group's. So the map is resolved once from the delta-synced category + list and applied to the month's categories. + + Two different things can send a category down the legacy fallback, and they + are counted and reported separately because they mean opposite things: + + * **Absent from the current category list** — routine. A historical month + names categories deleted since. A 24-month query produces these in bulk. + * **Present, but the group carries no `internal` flag** — a schema + regression. `internal` is `required` on `CategoryGroup` in spec 1.86.0, so + this should never happen, and it is the signal worth acting on. + + Collapsing both into one log event would bury the second under hundreds of + the first the moment multi-month analytics exist. + """ + + def __init__(self, groups: list[CategoryGroup]): + self._internal_by_category_id: dict[str, bool | None] = { + category.id: group.internal + for group in groups + for category in group.categories + } + self.unknown_category_count = 0 + self.missing_flag_count = 0 + + @property + def fallback_count(self) -> int: + """Total categories that took the legacy path, for either reason.""" + return self.unknown_category_count + self.missing_flag_count + + def is_internal(self, category: Category) -> bool: + """True if `category` belongs to a YNAB-internal group. + + A category whose group flag is unknown is not silently treated as "not + internal", which would readmit exactly what this class exists to + exclude. It falls back to the legacy group-name check. + """ + if category.id in self._internal_by_category_id: + group_internal = self._internal_by_category_id[category.id] + if group_internal is not None: + return group_internal + self.missing_flag_count += 1 + else: + self.unknown_category_count += 1 + + return (category.category_group_name or "") in LEGACY_INTERNAL_GROUP_NAMES + + def log_fallbacks(self) -> None: + """Report the *mechanism*, once per call. Never category names or amounts.""" + if self.unknown_category_count: + logger.info( + "%d categor%s absent from the current category map; " + "fell back to legacy group-name exclusion. This is expected for " + "months containing categories deleted since", + self.unknown_category_count, + "y is" if self.unknown_category_count == 1 else "ies are", + ) + if self.missing_flag_count: + logger.warning( + "CategoryGroup.internal missing for %d resolved categor%s; " + "falling back to legacy group-name exclusion. The field is " + "required by the YNAB API spec, so this indicates a schema change", + self.missing_flag_count, + "y" if self.missing_flag_count == 1 else "ies", + ) diff --git a/tests/test_internal_categories.py b/tests/test_internal_categories.py new file mode 100644 index 00000000..6d6ddb4a --- /dev/null +++ b/tests/test_internal_categories.py @@ -0,0 +1,279 @@ +"""Tests for group-level `internal` exclusion (§1.2). + +The regression this guards against: the category-level `internal` flag does not +inherit from the group. Credit Card Payment categories are `internal: false` +inside an `internal: true` group, so a category-level predicate readmits every +one of them — and their `activity` is the payment movement for spending already +counted in the real category, so credit-funded spending would be counted twice. +""" + +import json +from unittest.mock import AsyncMock, patch + +import pytest + +from src.models import Category, CategoryGroup, MonthDetail + + +def InternalCategoryResolver(groups): + """Imported lazily. + + `src/server/_shared.py` calls `sys.exit(1)` at import time when no token is + configured, and a module-level import of any `src.server` module would turn + that into a collection-time abort of the whole session. The rest of the suite + imports server modules inside test bodies for the same reason. + """ + from src.server.internal_categories import InternalCategoryResolver as _R + + return _R(groups) + +# The shape that makes a category-level predicate wrong: an internal group +# holding several payment categories that are themselves internal=false. The +# count is arbitrary — it only needs to be more than one, so that a predicate +# readmitting them is visibly readmitting a group rather than an edge case. +CREDIT_CARD_COUNT = 4 + + +def _cat(cat_id: str, group_id: str, group_name: str, **kw) -> Category: + return Category( + id=cat_id, + category_group_id=group_id, + category_group_name=group_name, + name=kw.pop("name", cat_id), + **kw, + ) + + +def _budget_shaped_groups() -> list[CategoryGroup]: + cc_cats = [ + _cat(f"cc-{i}", "grp-cc", "Credit Card Payments", internal=False, activity=-1000) + for i in range(CREDIT_CARD_COUNT) + ] + return [ + CategoryGroup( + id="grp-everyday", + name="Everyday", + internal=False, + categories=[ + _cat("cat-a", "grp-everyday", "Everyday", internal=False, activity=-123450) + ], + ), + CategoryGroup( + id="grp-cc", name="Credit Card Payments", internal=True, categories=cc_cats + ), + CategoryGroup( + id="grp-internal", + name="Internal Master Category", + internal=True, + categories=[ + _cat("inflow", "grp-internal", "Internal Master Category", + internal=True, activity=250000) + ], + ), + CategoryGroup( + id="grp-hidden", name="Hidden Categories", internal=True, categories=[] + ), + ] + + +class TestResolver: + def test_credit_card_categories_are_internal_via_their_group(self): + resolver = InternalCategoryResolver(_budget_shaped_groups()) + cc_cats = [ + c + for g in _budget_shaped_groups() + if g.id == "grp-cc" + for c in g.categories + ] + assert len(cc_cats) == CREDIT_CARD_COUNT + assert all(resolver.is_internal(c) for c in cc_cats) + # ...and the category-level flag would have said otherwise on every one. + assert all(c.internal is False for c in cc_cats) + + def test_user_categories_are_not_internal(self): + resolver = InternalCategoryResolver(_budget_shaped_groups()) + assert not resolver.is_internal( + _cat("cat-a", "grp-everyday", "Everyday", internal=False) + ) + + def test_hidden_categories_group_is_internal(self): + """The string match misses this group entirely; the flag catches it.""" + groups = [ + CategoryGroup( + id="grp-hidden", + name="Hidden Categories", + internal=True, + categories=[_cat("h-1", "grp-hidden", "Hidden Categories")], + ) + ] + resolver = InternalCategoryResolver(groups) + assert resolver.is_internal(groups[0].categories[0]) + + def test_survives_a_group_rename(self): + groups = [ + CategoryGroup( + id="grp-cc", + name="Tarjetas de crédito", + internal=True, + categories=[_cat("cc-1", "grp-cc", "Tarjetas de crédito")], + ) + ] + resolver = InternalCategoryResolver(groups) + assert resolver.is_internal(groups[0].categories[0]) + + def test_user_group_cannot_spoof_the_name(self): + """A user-created group named "Credit Card Payments" is not internal.""" + groups = [ + CategoryGroup( + id="grp-user", + name="Credit Card Payments", + internal=False, + categories=[_cat("u-1", "grp-user", "Credit Card Payments")], + ) + ] + resolver = InternalCategoryResolver(groups) + assert not resolver.is_internal(groups[0].categories[0]) + + +class TestUnresolvedGroups: + def test_unmapped_category_falls_back_to_the_legacy_name_check(self): + """A historical month can name a category absent from the current list.""" + resolver = InternalCategoryResolver(_budget_shaped_groups()) + assert resolver.is_internal(_cat("gone-1", "grp-gone", "Credit Card Payments")) + assert not resolver.is_internal(_cat("gone-2", "grp-gone", "Groceries")) + assert resolver.unknown_category_count == 2 + + def test_the_two_fallback_reasons_are_counted_separately(self): + """Routine historical churn must not be reported as a schema regression. + + A 24-month query produces unknown-category misses in bulk. If both + reasons shared one counter and one log event, a genuine missing-`internal` + regression would be buried under hundreds of expected misses. + """ + groups = _budget_shaped_groups() + groups.append( + CategoryGroup( + id="grp_no_flag", + name="Flagless", + categories=[_cat("nf-1", "grp_no_flag", "Flagless")], + ) + ) + resolver = InternalCategoryResolver(groups) + + resolver.is_internal(_cat("gone-1", "grp-gone", "Groceries")) + resolver.is_internal(_cat("nf-1", "grp_no_flag", "Flagless")) + + assert resolver.unknown_category_count == 1 + assert resolver.missing_flag_count == 1 + assert resolver.fallback_count == 2 + + def test_routine_misses_do_not_warn(self, caplog): + import logging + + caplog.set_level(logging.DEBUG) + resolver = InternalCategoryResolver(_budget_shaped_groups()) + for i in range(200): + resolver.is_internal(_cat(f"gone-{i}", "grp-gone", "Groceries")) + resolver.log_fallbacks() + + assert "absent from the current category map" in caplog.text + assert not [r for r in caplog.records if r.levelno >= logging.WARNING], ( + "expected historical churn was logged as a warning" + ) + + def test_missing_flag_warns(self, caplog): + import logging + + caplog.set_level(logging.DEBUG) + groups = [ + CategoryGroup( + id="grp_no_flag", + name="Flagless", + categories=[_cat("nf-1", "grp_no_flag", "Flagless")], + ) + ] + resolver = InternalCategoryResolver(groups) + resolver.is_internal(groups[0].categories[0]) + resolver.log_fallbacks() + + warnings = [r for r in caplog.records if r.levelno >= logging.WARNING] + assert len(warnings) == 1 + assert "indicates a schema change" in warnings[0].getMessage() + + def test_absent_internal_flag_falls_back(self): + """If a future API stops returning `internal`, absent must not read as + false — that would readmit every payment category.""" + groups = [ + CategoryGroup( + id="grp-cc", + name="Credit Card Payments", + categories=[_cat("cc-1", "grp-cc", "Credit Card Payments")], + ) + ] + assert groups[0].internal is None + resolver = InternalCategoryResolver(groups) + assert resolver.is_internal(groups[0].categories[0]) + assert resolver.fallback_count == 1 + + def test_fallback_logs_the_mechanism_only(self, caplog): + import logging + + caplog.set_level(logging.INFO) + resolver = InternalCategoryResolver(_budget_shaped_groups()) + resolver.is_internal(_cat("gone-1", "grp-gone", "Secret Vacation Fund")) + resolver.log_fallbacks() + + assert "legacy group-name exclusion" in caplog.text + assert "Secret Vacation Fund" not in caplog.text + assert "gone-1" not in caplog.text + + +class TestThroughTheAnalyticsTools: + """The regression that matters: exclusion must survive the wiring.""" + + @pytest.fixture + def analytics_cache(self): + with ( + patch("src.server._shared.cache", new_callable=AsyncMock) as mock, + patch("src.server._shared._ensure_db", new_callable=AsyncMock), + ): + mock.get_categories = AsyncMock(return_value=_budget_shaped_groups()) + yield mock + + def _month(self) -> MonthDetail: + cats = [c for g in _budget_shaped_groups() for c in g.categories] + # A historical category, absent from the current list, excluded via the + # legacy fallback. + cats.append( + _cat("gone-1", "grp-gone", "Credit Card Payments", activity=-45000) + ) + return MonthDetail(month="2026-08-01", income=250000, categories=cats) + + @pytest.mark.asyncio + async def test_spending_excludes_every_credit_card_category( + self, analytics_cache + ): + from src.server import get_spending_by_category + + analytics_cache.get_month = AsyncMock(return_value=self._month()) + result = json.loads( + await get_spending_by_category(plan_id="plan-1", month="2026-08-01") + ) + + assert {c["group"] for c in result["categories"]} == {"Everyday"} + assert result["total_spent"] == 123.45 + assert len(result["categories"]) == 1 + + @pytest.mark.asyncio + async def test_money_flow_excludes_every_credit_card_category( + self, analytics_cache + ): + from src.server import get_money_flow + + analytics_cache.get_month = AsyncMock(return_value=self._month()) + result = json.loads( + await get_money_flow(plan_id="plan-1", month="2026-08-01") + ) + + assert [n["name"] for n in result["nodes"]] == ["Income", "Everyday"] + assert result["total_spent"] == 123.45 diff --git a/tests/test_server.py b/tests/test_server.py index 68d434d1..f1ba482a 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -87,7 +87,12 @@ def _make_scheduled_transaction(**overrides) -> ScheduledTransaction: @pytest.fixture def mock_cache(): - with patch(CACHE_PATH) as mock, patch(DB_PATH, new_callable=AsyncMock): + # AsyncMock so a method a test does not stub is still awaitable. Analytics + # calls get_categories(); unstubbed it yields an empty category map, which is + # the unresolved-group path — the same path a historical month takes. + with patch(CACHE_PATH, new_callable=AsyncMock) as mock, patch( + DB_PATH, new_callable=AsyncMock + ): yield mock