From 4f18023284f53bd50aa2adb0e53651cf3e2397ab Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 29 Apr 2026 09:25:16 -0400 Subject: [PATCH] fix(journal): server-side guards on record_moment links + place names (#158) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Belt-and-suspenders to the prompt-layer changes in 6c309f1. Even when the model emits bogus task or place links, the server now refuses to persist them. ## Task auto-linking guard Reproducer (2026-04-27): a moment about restaging Docker on the swarm ended up with `task_ids: [2]` (Weston's ADHD Evaluation) — the only task in that day's prep. The model picked it up as filler. `_filter_task_ids_by_keyword_overlap` now runs after id resolution: it fetches each linked task's title, tokenizes both content and title through `_content_keywords` (lowercased, stopwords stripped, <3-char tokens dropped), and drops any link whose title shares no meaningful keyword with the moment content. The drop is logged at INFO so we can observe how often it fires post-deploy. The guard runs against the merged id list, so it covers both the preferred `task_titles` resolution path and the discouraged explicit `task_ids` path. ## Place placeholder guard Reproducer (2026-04-27): `place_names=["work"]` got passed to `record_moment`. "work" / "home" / "office" aren't places — they're role-labels for already-known geocoded locations. `_filter_placeholder_places` drops a small set of generic single-word labels before name resolution. Real user-named places that happen to be one word (e.g. "Akron") pass through. ## Tests 9 new unit tests in `tests/test_record_moment_guards.py` cover: - keyword tokenization & stopword stripping - placeholder place filtering (generic, case-insensitive, real-place pass-through) - keyword-overlap filtering (the exact 4/27 reproducer, the genuine- reference case, mixed/partial relevance, empty input) 13 tests pass; ruff clean. Closes Fable task #158. Co-Authored-By: Claude Opus 4.7 --- src/fabledassistant/services/tools/journal.py | 110 ++++++++++- tests/test_record_moment_guards.py | 181 ++++++++++++++++++ 2 files changed, 290 insertions(+), 1 deletion(-) create mode 100644 tests/test_record_moment_guards.py diff --git a/src/fabledassistant/services/tools/journal.py b/src/fabledassistant/services/tools/journal.py index eb4fde4..42c2133 100644 --- a/src/fabledassistant/services/tools/journal.py +++ b/src/fabledassistant/services/tools/journal.py @@ -3,6 +3,7 @@ from __future__ import annotations import datetime import logging +import re from typing import Any from sqlalchemy import func, select @@ -15,6 +16,97 @@ from fabledassistant.services.tools._registry import tool logger = logging.getLogger(__name__) +# Generic placeholders the model occasionally emits for `place_names` when no +# real place was named. Filtered out server-side as belt-and-suspenders to the +# prompt-layer guidance — these aren't places, just role-labels for locations +# the user already has named (Home / Work weather targets, etc.). +_PLACEHOLDER_PLACE_NAMES = frozenset({ + "work", "home", "office", "the office", "my office", "my work", + "my home", "house", "my house", "the house", +}) + +# Short closed-class words excluded from the keyword-overlap check below. +# Case is normalized to lowercase before comparison. +_STOPWORDS = frozenset({ + "the", "a", "an", "and", "or", "but", "at", "in", "on", "of", "to", + "for", "with", "by", "from", "about", "as", "is", "was", "were", "be", + "been", "being", "have", "has", "had", "do", "does", "did", "will", + "would", "could", "should", "may", "might", "must", "this", "that", + "these", "those", "i", "you", "he", "she", "we", "they", "it", "his", + "her", "their", "my", "your", "our", "its", "me", "him", "us", "them", + "are", "if", "so", "no", "not", "yes", "now", "then", "than", "too", + "very", "just", "also", "any", "all", "some", "one", "two", "out", + "up", "down", "off", "over", "under", "into", "onto", "upon", +}) + + +def _content_keywords(text: str) -> set[str]: + """Tokenize text into the meaningful keyword set used for overlap checks. + + Lowercased, alphanumeric runs only, stopwords removed, tokens shorter + than 3 chars dropped. Numeric tokens are kept (e.g. "Branch 14" yields + {"branch", "14"}) because they often anchor task references. + """ + tokens = re.split(r"[^a-z0-9]+", text.lower()) + return {t for t in tokens if len(t) >= 3 and t not in _STOPWORDS} + + +def _filter_placeholder_places(names: list[str]) -> tuple[list[str], list[str]]: + """Drop generic placeholders from a `place_names` list. + + Returns ``(kept, dropped)``. The dropped list is used purely for log + visibility — the moment is still created, the bogus links are just + not persisted. + """ + kept: list[str] = [] + dropped: list[str] = [] + for n in names: + norm = (n or "").strip() + if norm and norm.lower() in _PLACEHOLDER_PLACE_NAMES: + dropped.append(norm) + else: + kept.append(norm) + return kept, dropped + + +async def _filter_task_ids_by_keyword_overlap( + *, user_id: int, content: str, task_ids: list[int] +) -> list[int]: + """Drop task links whose title shares no meaningful keyword with the + moment content. + + Reproducer this guards against (2026-04-27): the model emitted + `task_titles=["Research Weston's ADHD Evaluation"]` on a moment about + restaging Docker — the title was real (Weston's task is in the prep + context) but the user never referenced it. Without this guard, the + only task surfaced in the prep gets attached to every moment as + filler. After this guard the link is dropped (zero-overlap) and a + log entry is emitted at INFO so we can observe how often this fires. + """ + if not task_ids: + return [] + async with async_session() as session: + stmt = select(Note.id, Note.title).where( + Note.user_id == user_id, + Note.id.in_(task_ids), + ) + rows = (await session.execute(stmt)).all() + title_by_id = {nid: (title or "") for nid, title in rows} + content_kw = _content_keywords(content) + kept: list[int] = [] + for tid in task_ids: + title = title_by_id.get(tid, "") + title_kw = _content_keywords(title) + if title_kw and content_kw & title_kw: + kept.append(tid) + else: + logger.info( + "record_moment: dropped task link id=%s title=%r — no keyword overlap with content %r", + tid, title, content[:80], + ) + return kept + + async def _resolve_entity_ids_by_name( *, user_id: int, @@ -180,10 +272,17 @@ async def record_moment_tool(*, user_id, arguments, conv_id=None, **_ctx): note_type="person", ) ) + raw_place_names = arguments.get("place_names") or [] + kept_place_names, dropped_place_names = _filter_placeholder_places(raw_place_names) + if dropped_place_names: + logger.info( + "record_moment: dropped placeholder place_names %r — not real places", + dropped_place_names, + ) place_ids.extend( await _resolve_entity_ids_by_name( user_id=user_id, - names=arguments.get("place_names") or [], + names=kept_place_names, note_type="place", ) ) @@ -208,6 +307,15 @@ async def record_moment_tool(*, user_id, arguments, conv_id=None, **_ctx): task_ids = list(dict.fromkeys(task_ids)) note_ids = list(dict.fromkeys(note_ids)) + # Drop task links that don't share a keyword with the moment content. + # Belt-and-suspenders to the prompt-layer rule "only link tasks the user + # explicitly references" — if the model attaches a task anyway (because + # it's in the prep context), the keyword check refuses to persist a link + # the moment can't justify. + task_ids = await _filter_task_ids_by_keyword_overlap( + user_id=user_id, content=content, task_ids=task_ids, + ) + moment = await create_moment( user_id=user_id, content=content, diff --git a/tests/test_record_moment_guards.py b/tests/test_record_moment_guards.py new file mode 100644 index 0000000..2772dcd --- /dev/null +++ b/tests/test_record_moment_guards.py @@ -0,0 +1,181 @@ +"""Tests for the record_moment server-side data-hygiene guards. + +These guard against two failure modes observed in real journal usage: + +1. The LLM emits ``task_titles`` referencing a task that's only in the + prep context — not actually mentioned by the user. Without a check, + every moment ends up linked to whatever's open in the user's queue. + +2. The LLM emits generic placeholder ``place_names`` like ``"work"`` / + ``"home"`` instead of real place notes. These role-labels aren't + places. + +Both are exercised through the pure helpers; full-stack handler tests +would require a session-bound DB fixture this suite doesn't have yet. +""" + +from unittest.mock import AsyncMock, patch + +import pytest + + +def test_content_keywords_drops_stopwords_and_short_tokens(): + from fabledassistant.services.tools.journal import _content_keywords + + kw = _content_keywords( + "I went to the store to pick up milk and bread for the kids." + ) + # Stopwords (the, to, for, and, i) gone; short tokens (kids? 4 chars - kept) + # filtered. Real content words kept. + assert "store" in kw + assert "milk" in kw + assert "bread" in kw + assert "kids" in kw + assert "the" not in kw + assert "to" not in kw + assert "for" not in kw + assert "i" not in kw + + +def test_content_keywords_extracts_named_entities(): + """Place names and project nouns survive tokenization. Short numeric + fragments (e.g. '14') get filtered alongside other <3-char tokens — + the surrounding alpha keywords carry the overlap weight in practice.""" + from fabledassistant.services.tools.journal import _content_keywords + + kw = _content_keywords("Migration prep at Branch 14 Bedford.") + assert "branch" in kw + assert "bedford" in kw + assert "migration" in kw + + +def test_filter_placeholder_places_drops_generic_role_labels(): + from fabledassistant.services.tools.journal import _filter_placeholder_places + + kept, dropped = _filter_placeholder_places( + ["work", "Famous Supply", "home", "Branch 14 Bedford", "the office"] + ) + assert "Famous Supply" in kept + assert "Branch 14 Bedford" in kept + assert "work" in dropped + assert "home" in dropped + assert "the office" in dropped + + +def test_filter_placeholder_places_is_case_insensitive(): + from fabledassistant.services.tools.journal import _filter_placeholder_places + + kept, dropped = _filter_placeholder_places(["WORK", "Home", "OFFICE"]) + assert kept == [] + assert set(dropped) == {"WORK", "Home", "OFFICE"} + + +def test_filter_placeholder_places_preserves_real_places_named_similarly(): + """A user-defined place that happens to be ONE word but isn't a generic + role-label (e.g. 'Akron') stays.""" + from fabledassistant.services.tools.journal import _filter_placeholder_places + + kept, dropped = _filter_placeholder_places(["Akron", "Cleveland"]) + assert kept == ["Akron", "Cleveland"] + assert dropped == [] + + +@pytest.mark.asyncio +async def test_keyword_overlap_drops_unrelated_task_link(): + """The reported failure mode: model attached Weston's ADHD task to a + Docker-swarm moment. With the keyword guard, the link gets dropped.""" + from fabledassistant.services.tools.journal import ( + _filter_task_ids_by_keyword_overlap, + ) + + # Mock the DB lookup to return the offending task title for id=2. + fake_session = AsyncMock() + fake_session.execute = AsyncMock() + fake_session.execute.return_value.all = lambda: [ + (2, "Research Weston's ADHD Evaluation"), + ] + fake_session.__aenter__ = AsyncMock(return_value=fake_session) + fake_session.__aexit__ = AsyncMock(return_value=None) + + with patch( + "fabledassistant.services.tools.journal.async_session", + return_value=fake_session, + ): + kept = await _filter_task_ids_by_keyword_overlap( + user_id=1, + content="Working on restaging Docker in the swarm; encountered network breakage on a Windows node.", + task_ids=[2], + ) + + assert kept == [] + + +@pytest.mark.asyncio +async def test_keyword_overlap_keeps_genuinely_referenced_task(): + """When the moment content shares a meaningful keyword with the task + title, the link is preserved.""" + from fabledassistant.services.tools.journal import ( + _filter_task_ids_by_keyword_overlap, + ) + + fake_session = AsyncMock() + fake_session.execute = AsyncMock() + fake_session.execute.return_value.all = lambda: [ + (5, "Restage Docker on Fam-dockerwin04"), + ] + fake_session.__aenter__ = AsyncMock(return_value=fake_session) + fake_session.__aexit__ = AsyncMock(return_value=None) + + with patch( + "fabledassistant.services.tools.journal.async_session", + return_value=fake_session, + ): + kept = await _filter_task_ids_by_keyword_overlap( + user_id=1, + content="Reinstalled Microsoft container support; Docker restage now works.", + task_ids=[5], + ) + + # 'docker' appears in both → keep + assert kept == [5] + + +@pytest.mark.asyncio +async def test_keyword_overlap_handles_partial_relevance(): + """Mixed ids: keep the related one, drop the unrelated one.""" + from fabledassistant.services.tools.journal import ( + _filter_task_ids_by_keyword_overlap, + ) + + fake_session = AsyncMock() + fake_session.execute = AsyncMock() + fake_session.execute.return_value.all = lambda: [ + (2, "Research Weston's ADHD Evaluation"), + (5, "Restage Docker on Fam-dockerwin04"), + ] + fake_session.__aenter__ = AsyncMock(return_value=fake_session) + fake_session.__aexit__ = AsyncMock(return_value=None) + + with patch( + "fabledassistant.services.tools.journal.async_session", + return_value=fake_session, + ): + kept = await _filter_task_ids_by_keyword_overlap( + user_id=1, + content="Working on restaging Docker in the swarm.", + task_ids=[2, 5], + ) + + assert kept == [5] + + +@pytest.mark.asyncio +async def test_keyword_overlap_empty_task_ids_returns_empty(): + from fabledassistant.services.tools.journal import ( + _filter_task_ids_by_keyword_overlap, + ) + + kept = await _filter_task_ids_by_keyword_overlap( + user_id=1, content="anything", task_ids=[], + ) + assert kept == []