Retrieval: the passage that matched, every kind searchable, work logs and charters findable #175
@@ -11,6 +11,7 @@ import time
|
||||
|
||||
from scribe.mcp._context import current_user_id
|
||||
from scribe.services.access import owner_names_for
|
||||
from scribe.services.knowledge import content_type_filters
|
||||
from scribe.services.text import MATCHED_PASSAGE, excerpt_fields
|
||||
from scribe.services.embeddings import (
|
||||
DEFAULT_SIMILARITY_THRESHOLD, semantic_search_milestones, semantic_search_notes,
|
||||
@@ -28,6 +29,18 @@ from scribe.services.retrieval_telemetry import record_retrieval, retrieval_summ
|
||||
_EXCERPT_CHARS = 1000
|
||||
|
||||
|
||||
# The kinds `content_type` accepts are DERIVED from the facet table, not listed
|
||||
# again here — that table is where a kind is declared (#3161), and a second
|
||||
# hand-kept copy in this module is precisely how the agent's door came to offer
|
||||
# two kinds while browse offered nine (#4250). `content_type_filters` carries
|
||||
# the mapping, the 'all'/'note' special cases and the refusal.
|
||||
#
|
||||
# These two do not reach `semantic_search_notes` at all: they have their own
|
||||
# search and their own result shape, so they are dispatched before the mapping
|
||||
# and passed in only so the refusal message lists everything THIS door takes.
|
||||
_OWN_SEARCH = ("rule", "milestone")
|
||||
|
||||
|
||||
async def _search_rules(uid: int, q: str, limit: int, project_id: int) -> dict:
|
||||
"""Rules by meaning — a separate result shape because a rule IS different.
|
||||
|
||||
@@ -170,8 +183,28 @@ async def search(
|
||||
|
||||
Args:
|
||||
q: search query string.
|
||||
content_type: 'all' (default), 'note' (notes only), 'task' (tasks
|
||||
only), or 'rule' (RULES only — the operator's standing
|
||||
content_type: which kind of record to search. 'all' (default) spans
|
||||
every note and task.
|
||||
|
||||
THE BROAD TWO: 'note' is any non-task record — it still includes
|
||||
snippets, lessons and processes, so it means "knowledge, not work
|
||||
items". 'task' is any task whatever its kind.
|
||||
|
||||
THE SPECIFIC KINDS, each narrowing to one: 'snippet' (recorded
|
||||
prior art — reach for this BEFORE writing a helper, rather than
|
||||
searching 'all' and reading past the issues), 'lesson' (a
|
||||
transferable insight, the kind that exists to be recalled by
|
||||
situation), 'process' (a stored procedure the operator saved),
|
||||
'issue' (corrective work — "has this already been reported?"),
|
||||
'spike' (a time-boxed investigation, whose output is an answer),
|
||||
'work', and 'plan' (retired; the ~90 legacy plan-tasks).
|
||||
|
||||
An unrecognised value is REFUSED with the list of valid ones
|
||||
rather than quietly returning nothing: an empty result set is a
|
||||
claim that the corpus holds nothing, and a typo must not be able
|
||||
to make that claim.
|
||||
|
||||
Or 'rule' (RULES only — the operator's standing
|
||||
instructions, searchable by meaning since milestone 307).
|
||||
Reach for 'rule' when you want to know whether a standing
|
||||
instruction covers something: "is there a rule about release
|
||||
@@ -225,11 +258,12 @@ async def search(
|
||||
return await _search_rules(uid, q, limit, project_id)
|
||||
if content_type == "milestone":
|
||||
return await _search_milestones(uid, q, limit, project_id)
|
||||
is_task = {"note": False, "task": True}.get(content_type) # None => any
|
||||
filters = content_type_filters(content_type, extra=_OWN_SEARCH)
|
||||
is_task = filters.get("is_task")
|
||||
t0 = time.perf_counter()
|
||||
report: dict = {}
|
||||
raw = await semantic_search_notes(
|
||||
uid, q, limit=limit, is_task=is_task,
|
||||
uid, q, limit=limit, **filters,
|
||||
project_id=project_id or None,
|
||||
system_id=system_id or None,
|
||||
# A LESSON is reachable from any project (milestone 385). The kind
|
||||
|
||||
+13
-12
@@ -8,6 +8,7 @@ from scribe.services.embeddings import (
|
||||
INTERACTIVE_SEARCH_THRESHOLD as _REST_SEARCH_THRESHOLD,
|
||||
)
|
||||
from scribe.services.embeddings import semantic_search_notes
|
||||
from scribe.services.knowledge import content_type_filters
|
||||
from scribe.services.retrieval_telemetry import record_retrieval
|
||||
|
||||
# The interactive floor lives in embeddings.py now, shared with Browse search
|
||||
@@ -16,15 +17,6 @@ from scribe.services.retrieval_telemetry import record_retrieval
|
||||
search_bp = Blueprint("search", __name__, url_prefix="/api/search")
|
||||
|
||||
|
||||
def _content_type_to_is_task(content_type: str) -> bool | None:
|
||||
"""Map content_type query param to semantic_search_notes is_task arg."""
|
||||
if content_type == "note":
|
||||
return False
|
||||
if content_type == "task":
|
||||
return True
|
||||
return None # "all" or unknown → no filter
|
||||
|
||||
|
||||
@search_bp.route("", methods=["GET"])
|
||||
@login_required
|
||||
async def search_route():
|
||||
@@ -33,9 +25,18 @@ async def search_route():
|
||||
if not q:
|
||||
return jsonify({"error": "q is required"}), 400
|
||||
|
||||
content_type = request.args.get("content_type", "all")
|
||||
limit = min(request.args.get("limit", 10, type=int), 50)
|
||||
is_task = _content_type_to_is_task(content_type)
|
||||
# Every kind the facet table declares, derived rather than mapped here —
|
||||
# this route used to know exactly two and read anything else as "no
|
||||
# filter", so `?content_type=snippets` silently returned the whole corpus
|
||||
# (#4250). An unknown kind is now a 400 naming the ones that exist: a
|
||||
# result set is an answer, and it should not be able to answer a question
|
||||
# nobody asked.
|
||||
try:
|
||||
filters = content_type_filters(request.args.get("content_type", "all"))
|
||||
except ValueError as exc:
|
||||
return jsonify({"error": str(exc)}), 400
|
||||
is_task = filters.get("is_task")
|
||||
# Same association filters the MCP tool takes (#33). Optional, default
|
||||
# global: this route has NO frontend consumer today (measured 2026-08-08 —
|
||||
# the web UI searches through /api/knowledge), so it serves API callers,
|
||||
@@ -46,7 +47,7 @@ async def search_route():
|
||||
t0 = time.perf_counter()
|
||||
report: dict = {}
|
||||
results = await semantic_search_notes(
|
||||
uid, q, limit=limit, is_task=is_task, threshold=_REST_SEARCH_THRESHOLD,
|
||||
uid, q, limit=limit, **filters, threshold=_REST_SEARCH_THRESHOLD,
|
||||
project_id=project_id, system_id=system_id,
|
||||
# The user typed this, so it reaches everything they may read.
|
||||
scope="read",
|
||||
|
||||
@@ -376,6 +376,69 @@ def matches_facet(note, note_type: str | None) -> bool:
|
||||
return not note.is_task and note.note_type == value
|
||||
|
||||
|
||||
def search_filters_for(facet: str) -> dict:
|
||||
"""The semantic-search kwargs one facet implies: is_task + note_type/task_kind.
|
||||
|
||||
A third dialect of the same table, for the arm that has no SQL statement to
|
||||
narrow and no fetched row to test — it is passing filters INTO
|
||||
`semantic_search_notes`. `_apply_type_filter` is the SQL dialect and
|
||||
`matches_facet` the Python one; all three read `_FACETS`, which is what
|
||||
keeps "adding a kind" a single edit (#3161).
|
||||
|
||||
Returns kwargs rather than a tuple so a caller splats it and cannot pair
|
||||
`task_kind` with `is_task=False` by writing the positions out of order.
|
||||
"""
|
||||
is_task, value = _facet(facet)
|
||||
if is_task:
|
||||
return {"is_task": True, "task_kind": value}
|
||||
return {"is_task": False, "note_type": value}
|
||||
|
||||
|
||||
# The two names a `content_type` parameter carries that are not facets. Both
|
||||
# search doors that take one — the MCP tool and /api/search — have always
|
||||
# spelled them this way, so they are the contract rather than a convenience:
|
||||
#
|
||||
# 'all' (and empty) — no kind filter at all.
|
||||
# 'note' — ANY non-task record, so snippets, lessons and processes
|
||||
# are all still in scope. The BROWSE facet of the same
|
||||
# name is narrower (`note_type == 'note'` exactly). The
|
||||
# two are deliberately different: narrowing this one
|
||||
# would stop returning snippets to every caller that
|
||||
# already asks this way, and the doors document the
|
||||
# containment instead.
|
||||
_CONTENT_TYPE_BROAD = {
|
||||
"": {"is_task": None},
|
||||
"all": {"is_task": None},
|
||||
"note": {"is_task": False},
|
||||
}
|
||||
|
||||
|
||||
def content_type_filters(content_type: str, extra: tuple = ()) -> dict:
|
||||
"""The search kwargs a door's `content_type` parameter implies.
|
||||
|
||||
Lives here, beside `_FACETS`, because a door that keeps its own map is how
|
||||
this went wrong: the agent's search offered two kinds and browse offered
|
||||
nine, and a third copy in `/api/search` offered two more quietly still
|
||||
(#4250). Derived, so adding a kind stays one edit (#3161).
|
||||
|
||||
Raises on anything unrecognised. `_facet` alone would fall back to reading
|
||||
an unknown string as a note_type, which matches no row — so a typo comes
|
||||
back as a confident empty result, and an empty result is a CLAIM that the
|
||||
corpus holds nothing of the sort. A door has to be able to say "that is not
|
||||
a kind" instead. `extra` names kinds the caller dispatches elsewhere (the
|
||||
MCP tool's 'rule' and 'milestone'), so the refusal lists what that door
|
||||
really accepts and not a vocabulary from some other door.
|
||||
"""
|
||||
if content_type in _CONTENT_TYPE_BROAD:
|
||||
return dict(_CONTENT_TYPE_BROAD[content_type])
|
||||
if content_type in FACET_TYPES:
|
||||
return search_filters_for(content_type)
|
||||
raise ValueError(
|
||||
f"unknown content_type {content_type!r}. Valid: "
|
||||
+ ", ".join(sorted({"all", *FACET_TYPES, *extra}))
|
||||
)
|
||||
|
||||
|
||||
def _apply_type_filter(stmt, note_type: str | None):
|
||||
"""Apply the type facet to a Note select. Trashed rows are always excluded."""
|
||||
stmt = stmt.where(Note.deleted_at.is_(None))
|
||||
|
||||
@@ -125,22 +125,107 @@ async def test_a_short_record_arrives_whole_and_unmarked():
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_fable_search_content_type_filters_at_service_layer():
|
||||
"""content_type maps to the is_task kwarg passed to the service."""
|
||||
"""The three historical values keep meaning exactly what they meant.
|
||||
|
||||
`note` is the one that could plausibly have been tightened when the
|
||||
specific kinds arrived — it is BROAD here (any non-task, snippets and
|
||||
lessons included) while the web facet of the same name is narrow. Pinning
|
||||
it stops a later tidy-up from silently removing snippets from every caller
|
||||
that already asks this way (#4250)."""
|
||||
_user_id_ctx.set(7)
|
||||
mock_search = AsyncMock(return_value=[])
|
||||
with patch("scribe.mcp.tools.search.semantic_search_notes", mock_search):
|
||||
await search(q="x", content_type="task")
|
||||
assert mock_search.call_args.kwargs["is_task"] is True
|
||||
assert mock_search.call_args.kwargs.get("task_kind") is None
|
||||
|
||||
mock_search.reset_mock()
|
||||
await search(q="x", content_type="note")
|
||||
assert mock_search.call_args.kwargs["is_task"] is False
|
||||
assert mock_search.call_args.kwargs.get("note_type") is None
|
||||
|
||||
mock_search.reset_mock()
|
||||
await search(q="x", content_type="all")
|
||||
assert mock_search.call_args.kwargs["is_task"] is None
|
||||
|
||||
|
||||
# --- the specific kinds: the engine already supported them (#4250) -----------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
"content_type,expected",
|
||||
[
|
||||
("snippet", {"is_task": False, "note_type": "snippet"}),
|
||||
("lesson", {"is_task": False, "note_type": "lesson"}),
|
||||
("process", {"is_task": False, "note_type": "process"}),
|
||||
("issue", {"is_task": True, "task_kind": "issue"}),
|
||||
("spike", {"is_task": True, "task_kind": "spike"}),
|
||||
("work", {"is_task": True, "task_kind": "work"}),
|
||||
("plan", {"is_task": True, "task_kind": "plan"}),
|
||||
],
|
||||
)
|
||||
async def test_each_specific_kind_reaches_the_engine_filter_it_names(
|
||||
content_type, expected
|
||||
):
|
||||
"""`note_type` and `task_kind` were parameters of the search all along —
|
||||
what was missing was a way to ask for them from the agent's door. Asking
|
||||
for a snippet must narrow to snippets, not merely to non-tasks."""
|
||||
_user_id_ctx.set(7)
|
||||
mock_search = AsyncMock(return_value=[])
|
||||
with patch("scribe.mcp.tools.search.semantic_search_notes", mock_search):
|
||||
await search(q="x", content_type=content_type)
|
||||
kwargs = mock_search.call_args.kwargs
|
||||
for key, value in expected.items():
|
||||
assert kwargs[key] == value, f"{content_type}: {key}"
|
||||
|
||||
|
||||
def test_the_vocabulary_is_derived_from_the_facet_table_not_recopied():
|
||||
"""#3161's property, held at this door too: adding a kind to `_FACETS` is
|
||||
one edit. A hand-kept list here is exactly how the agent's search came to
|
||||
offer two kinds while the web's offered nine."""
|
||||
from scribe.services.knowledge import FACET_TYPES, content_type_filters
|
||||
|
||||
for facet in FACET_TYPES:
|
||||
filters = content_type_filters(facet) # raises if a kind is unreachable
|
||||
assert "is_task" in filters, facet
|
||||
|
||||
# The guard can fail: a name absent from the table is refused, so this is
|
||||
# membership in the table and not "every string works" (#167).
|
||||
with pytest.raises(ValueError):
|
||||
content_type_filters("a-kind-that-is-not-in-the-facet-table")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_an_unknown_content_type_is_refused_rather_than_returning_nothing():
|
||||
"""An empty result set is a CLAIM — "the corpus holds nothing like this" —
|
||||
and an agent acts on it by writing the thing it could not find. A typo must
|
||||
not be able to make that claim, so the door raises with the vocabulary
|
||||
instead of falling through to a filter that matches no row."""
|
||||
_user_id_ctx.set(7)
|
||||
mock_search = AsyncMock(return_value=[])
|
||||
with patch("scribe.mcp.tools.search.semantic_search_notes", mock_search):
|
||||
with pytest.raises(ValueError) as err:
|
||||
await search(q="x", content_type="snippets") # plural typo
|
||||
mock_search.assert_not_awaited()
|
||||
message = str(err.value)
|
||||
assert "snippets" in message
|
||||
assert "snippet" in message and "lesson" in message # names the valid ones
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_the_docstring_names_every_kind_the_tool_accepts():
|
||||
"""The docstring IS the agent-facing contract (#2846) — a filter an agent
|
||||
has not been told about is unreachable however well it is wired."""
|
||||
from scribe.mcp.tools.search import search as search_tool
|
||||
from scribe.services.knowledge import FACET_TYPES
|
||||
|
||||
doc = search_tool.__doc__ or ""
|
||||
for facet in FACET_TYPES:
|
||||
assert f"'{facet}'" in doc, f"{facet} is accepted but never documented"
|
||||
assert "'rule'" in doc and "'milestone'" in doc and "'all'" in doc
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_an_explicit_search_reaches_a_lesson_from_any_project():
|
||||
"""The wiring half of milestone 385 step 3.
|
||||
|
||||
+42
-10
@@ -1,18 +1,50 @@
|
||||
"""Unit tests for the search route parameter mapping."""
|
||||
from scribe.routes.search import _content_type_to_is_task
|
||||
"""/api/search — the kind filter it offers, and what it does with a bad one.
|
||||
|
||||
This route used to carry its own two-value map (`note`/`task`/everything else
|
||||
is `all`), which made it the third hand-kept copy of a vocabulary that lives in
|
||||
one table. Both halves of #4250 are pinned here: every declared kind is
|
||||
reachable, and a kind that does not exist is refused rather than widened.
|
||||
"""
|
||||
import pytest
|
||||
|
||||
from scribe.services.knowledge import FACET_TYPES, content_type_filters
|
||||
|
||||
|
||||
def test_content_type_note():
|
||||
assert _content_type_to_is_task("note") is False
|
||||
def test_the_three_historical_values_still_mean_what_they_meant():
|
||||
assert content_type_filters("all") == {"is_task": None}
|
||||
assert content_type_filters("task")["is_task"] is True
|
||||
# BROAD on purpose: any non-task, so snippets and lessons stay in scope.
|
||||
assert content_type_filters("note") == {"is_task": False}
|
||||
|
||||
|
||||
def test_content_type_task():
|
||||
assert _content_type_to_is_task("task") is True
|
||||
@pytest.mark.parametrize("facet", sorted(FACET_TYPES))
|
||||
def test_every_declared_kind_is_reachable_through_this_door(facet):
|
||||
"""The engine took `note_type` and `task_kind` all along — the route was
|
||||
simply unable to say them."""
|
||||
filters = content_type_filters(facet)
|
||||
assert "is_task" in filters
|
||||
if facet not in ("all", "note", "task"):
|
||||
assert filters.get("note_type") == facet or filters.get("task_kind") == facet
|
||||
|
||||
|
||||
def test_content_type_all():
|
||||
assert _content_type_to_is_task("all") is None
|
||||
def test_an_unknown_kind_is_refused_instead_of_silently_widening():
|
||||
"""The old map read anything unrecognised as "no filter", so
|
||||
`?content_type=snippets` returned the whole corpus while looking like a
|
||||
narrowed search — the failure mode that answers a question nobody asked."""
|
||||
with pytest.raises(ValueError) as err:
|
||||
content_type_filters("snippets")
|
||||
assert "snippets" in str(err.value)
|
||||
assert "snippet" in str(err.value) # names the real ones
|
||||
|
||||
|
||||
def test_content_type_unknown_defaults_to_all():
|
||||
assert _content_type_to_is_task("unknown") is None
|
||||
def test_a_doors_refusal_lists_only_what_that_door_accepts():
|
||||
"""`rule` and `milestone` are the MCP tool's own searches. This route has
|
||||
neither, so offering them in its error would send a caller at a parameter
|
||||
that does not work here."""
|
||||
with pytest.raises(ValueError) as plain:
|
||||
content_type_filters("nonsense")
|
||||
assert "rule" not in str(plain.value)
|
||||
|
||||
with pytest.raises(ValueError) as extra:
|
||||
content_type_filters("nonsense", extra=("rule", "milestone"))
|
||||
assert "rule" in str(extra.value) and "milestone" in str(extra.value)
|
||||
|
||||
Reference in New Issue
Block a user