fix(journal): server-side guards on record_moment links + place names (#158)
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user