fix(search): show the passage that matched, not the opening of the body (#4243)
CI & Build / Plugin hooks (push) Successful in 10s
CI & Build / Python lint (push) Successful in 3s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Failing after 1m5s
CI & Build / Build & push image (push) Skipped
CI & Build / integration (push) Successful in 46s
CI & Build / Plugin hooks (push) Successful in 10s
CI & Build / Python lint (push) Successful in 3s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Failing after 1m5s
CI & Build / Build & push image (push) Skipped
CI & Build / integration (push) Successful in 46s
Raised by the operator: are we limiting what comes back by character count,
and how do we verify the pertinent part is the part displayed?
We were not. mcp/tools/search.py sent (note.body or "")[:240] — a head cut,
with no marker that anything had been removed, so a 240-character preview of
a 4000-character record was indistinguishable from a complete short one.
The opening is the wrong span. The match is semantic and per chunk, and
semantic_search_notes collapses to best-chunk-per-note — its own comment at
the collapse says "the first appearance of a note is its best chunk". So the
system identified the passage that earned the hit and then discarded it:
select(Note, distance) kept no chunk column. A record could rank first on its
sixth paragraph, be previewed by its first, and be judged irrelevant on a
span the search had already scored lower. That biases against long records,
and it is self-concealing — the caller who does not open it never learns the
preview was misleading.
- embeddings: chunk_index/chunk_text ride along in the select, and the
collapse records the winner in report["best_chunk"]. Carried in `report`,
NOT by widening the return tuple: ten callers unpack (score, note) at
~18 sites and nothing would catch the misses (lesson #4207). `report` is
the side-channel this function already uses for best_available_score.
- search(): excerpt / excerpt_is / body_length, and read_full when there is
more. A caller that cannot tell a matched passage from a document opening
cannot judge whether to look deeper, which is the only decision the field
supports.
elide() moves to services/text.py so both callers share one copy, and it
keeps BOTH ends with a stated gap — it is the fallback for when nothing
identifies a better span than "all of it", not the goal.
Also fixes a guard that produced a false failure on the previous commit:
test_pull_telemetry checked `"project_id: int = 0" in body.split("\n")[0]`,
which sees only the first line, so wrapping get_task's signature over four
lines made it report a function that does take the project as one that does
not. Parsed with ast now, and proven to still reject an absent or
wrongly-typed parameter rather than being appeased by reflowing the code.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
@@ -20,6 +20,15 @@ from scribe.mcp.tools.tasks import (
|
||||
elide, get_task, list_tasks, work_log_payload,
|
||||
)
|
||||
from scribe.services.notes import brief_row
|
||||
# Bound at IMPORT time, which is before any fixture runs — so these names
|
||||
# keep pointing at the real implementations even though conftest's autouse
|
||||
# _no_task_log_arm replaces the module attributes for every test. Reaching
|
||||
# them as `task_logs.logs_for_task` would get the stub and test nothing.
|
||||
from scribe.services.task_logs import (
|
||||
count_logs_for_task,
|
||||
log_counts_for_tasks,
|
||||
logs_for_task,
|
||||
)
|
||||
from tests.helpers import fake_task, make_mock_session
|
||||
|
||||
pytestmark = pytest.mark.usefixtures("_bind_user")
|
||||
@@ -304,8 +313,6 @@ async def test_logs_for_task_is_not_filtered_by_who_wrote_the_entry():
|
||||
"""`list_logs` filters TaskLog.user_id == user_id, which hands a shared
|
||||
collaborator an empty list that reads as "no work has been done". The work
|
||||
log belongs to the task."""
|
||||
from scribe.services import task_logs as svc
|
||||
|
||||
session = make_mock_session()
|
||||
session.execute = AsyncMock(return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
@@ -314,7 +321,7 @@ async def test_logs_for_task_is_not_filtered_by_who_wrote_the_entry():
|
||||
AsyncMock(return_value=True)), \
|
||||
patch("scribe.services.task_logs.async_session",
|
||||
MagicMock(return_value=session)):
|
||||
await svc.logs_for_task(7, 42)
|
||||
await logs_for_task(7, 42)
|
||||
stmt = str(session.execute.await_args.args[0])
|
||||
assert "task_logs.task_id" in stmt
|
||||
assert "task_logs.user_id" not in stmt
|
||||
@@ -324,26 +331,22 @@ async def test_logs_for_task_is_not_filtered_by_who_wrote_the_entry():
|
||||
async def test_logs_for_task_refuses_a_task_the_caller_cannot_read():
|
||||
"""Unscoped would have been the mirror-image hole: rule #78 is about
|
||||
routing the question through the access layer, in both directions."""
|
||||
from scribe.services import task_logs as svc
|
||||
|
||||
opened = MagicMock()
|
||||
with patch("scribe.services.task_logs.can_read_note",
|
||||
AsyncMock(return_value=False)), \
|
||||
patch("scribe.services.task_logs.async_session", opened):
|
||||
out = await svc.logs_for_task(7, 42)
|
||||
out = await logs_for_task(7, 42)
|
||||
assert out == []
|
||||
assert opened.call_count == 0
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_count_for_an_unreadable_task_is_zero_not_a_leak():
|
||||
from scribe.services import task_logs as svc
|
||||
|
||||
opened = MagicMock()
|
||||
with patch("scribe.services.task_logs.can_read_note",
|
||||
AsyncMock(return_value=False)), \
|
||||
patch("scribe.services.task_logs.async_session", opened):
|
||||
assert await svc.count_logs_for_task(7, 42) == 0
|
||||
assert await count_logs_for_task(7, 42) == 0
|
||||
assert opened.call_count == 0
|
||||
|
||||
|
||||
@@ -351,15 +354,13 @@ async def test_count_for_an_unreadable_task_is_zero_not_a_leak():
|
||||
async def test_log_counts_for_tasks_scopes_by_readability_in_the_same_query():
|
||||
"""A per-row can_read_note would be the N+1 this function exists to avoid,
|
||||
so the permission goes in as set membership instead."""
|
||||
from scribe.services import task_logs as svc
|
||||
|
||||
session = make_mock_session()
|
||||
session.execute = AsyncMock(return_value=MagicMock(
|
||||
all=MagicMock(return_value=[(1, 3)])
|
||||
))
|
||||
with patch("scribe.services.task_logs.async_session",
|
||||
MagicMock(return_value=session)):
|
||||
out = await svc.log_counts_for_tasks(7, [1, 2])
|
||||
out = await log_counts_for_tasks(7, [1, 2])
|
||||
assert out == {1: 3}
|
||||
stmt = str(session.execute.await_args.args[0])
|
||||
assert "notes" in stmt.lower()
|
||||
@@ -371,5 +372,5 @@ async def test_no_ids_asks_the_database_nothing():
|
||||
|
||||
opened = MagicMock()
|
||||
with patch("scribe.services.task_logs.async_session", opened):
|
||||
assert await svc.log_counts_for_tasks(7, []) == {}
|
||||
assert await log_counts_for_tasks(7, []) == {}
|
||||
assert opened.call_count == 0
|
||||
|
||||
Reference in New Issue
Block a user