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 == []