diff --git a/src/scribe/mcp/tools/search.py b/src/scribe/mcp/tools/search.py index f22b2eb..6f05632 100644 --- a/src/scribe/mcp/tools/search.py +++ b/src/scribe/mcp/tools/search.py @@ -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 diff --git a/src/scribe/routes/search.py b/src/scribe/routes/search.py index b4303f5..4bc8f9f 100644 --- a/src/scribe/routes/search.py +++ b/src/scribe/routes/search.py @@ -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", diff --git a/src/scribe/services/knowledge.py b/src/scribe/services/knowledge.py index 8e053c9..efb0ea8 100644 --- a/src/scribe/services/knowledge.py +++ b/src/scribe/services/knowledge.py @@ -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)) diff --git a/tests/test_mcp_tool_search.py b/tests/test_mcp_tool_search.py index 88d4ee1..ee167c1 100644 --- a/tests/test_mcp_tool_search.py +++ b/tests/test_mcp_tool_search.py @@ -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. diff --git a/tests/test_search_route.py b/tests/test_search_route.py index 61b6465..d5332d3 100644 --- a/tests/test_search_route.py +++ b/tests/test_search_route.py @@ -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)