From f8e53c1c3575a6dcc0859863a117659ab1bbe0ff Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 08:05:52 -0400 Subject: [PATCH 1/9] fix(telemetry): a warning fired on an arm whose decline rate is arithmetic, not evidence (#4232) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by reading a live `retrieval_telemetry` readout after milestone 419 deployed, not by inspection. The readout said: cannot_decline / report_preference — "45 calls, 0 of them returned nothing. An arm that fires unasked has to be able to say nothing; this one never has. Check that it applies its floor at all." And printed, beside it, that arm's band: p10 = p50 = p90 = min = max = 0.791. FIVE IDENTICAL PERCENTILES IS THE TELL. That is not a ranking, it is one record at one score on every call — because `report_preference` searches a fixed string (`reply_preferences.COMPLETION_QUERY`, a module constant, and deliberately so). For a fixed query against a stable corpus the top score is a CONSTANT, so the arm's decline rate is 0% or 100% and never in between; which of the two it is depends only on where the bar sits relative to that one number. "Never returned nothing" is therefore arithmetic, not evidence, and the warning's own remedy — check whether it applies a floor — cannot be answered from it. The arm already knew this about itself; the warning did not: "a fixed query makes this arm's score a constant and a floor a hair above it produces a dead arm no amount of traffic will ever reveal" — services/reply_preferences.py THIS CLASS OF BUG ALREADY HAS A GUARD, which is the argument for the shape of the fix. `Point.logs_unconditionally` exists because of #3497: both rule arms once logged only their hits, so their zero count was structurally 0 and this same warning would have fired on a LOGGING property while sending the reader to move a threshold that was never involved. This is that one step over — a QUERY-SHAPE property — and gets the same treatment: a declared field on `Point`, and exclusion rather than trust. AND THE WARNING THAT WOULD BE INFORMATIVE HERE DID NOT EXIST. For a fixed-query arm the dangerous state is the mirror image: every call empty, meaning the bar is above the constant and no further traffic will ever move it. The arm is off rather than quiet, and nothing in the readout said so — `expects_traffic` covers an arm with NO calls, not one with calls and a 100% decline rate. That state is real and reached: `report_preference` once logged 69 consecutive declines at 0.0006 under its bar. So `fixed_query_never_clears` sends the reader to `near_miss_samples` and not to the dial — because that incident is also the one where the statistic and the correct action pointed opposite ways. Every percentile said lower the floor; opening the refused record showed it was rule 77 arriving as a false positive, and lowering it would have delivered that rule on every completion report ever written. Guards in tests/test_retrieval_warnings.py, including the falsifier that matters most here: `cannot_decline` must still fire on an arm whose query varies, or this change is a disabled check wearing a narrowed one's clothes. Both boundaries tested from both sides, per that module's own standard. The new code is documented in the `retrieval_telemetry` tool docstring beside the others (rule 33) — an undocumented code in a readout is a reader meeting a verdict with no way to disagree with it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/mcp/tools/search.py | 14 +++- src/scribe/services/retrieval_registry.py | 30 ++++++++- src/scribe/services/retrieval_telemetry.py | 50 ++++++++++++++ tests/test_retrieval_warnings.py | 77 ++++++++++++++++++++++ 4 files changed, 169 insertions(+), 2 deletions(-) diff --git a/src/scribe/mcp/tools/search.py b/src/scribe/mcp/tools/search.py index ff643b1..4623ca9 100644 --- a/src/scribe/mcp/tools/search.py +++ b/src/scribe/mcp/tools/search.py @@ -421,7 +421,19 @@ It is an UPPER BOUND per surface: a pull records the door it came Only ever raised for unbidden arms known to log unconditionally: a search returning a list every time is doing its job, and an arm whose zeros were never written would flag a LOGGING bug while pointing you at - a threshold, which is #3497 exactly. + a threshold, which is #3497 exactly. Nor for an arm whose query never + changes — see the next entry. + - `fixed_query_never_clears` — an arm that always searches the SAME query + returned nothing on every call. Its score is one constant, so this is + not a quiet window: the bar sits above that constant and no amount of + further traffic will produce a different result. The arm is off rather + than silent, and nothing else here would say so. The same property is + why `cannot_decline` is not raised for these arms: with a constant + score the decline rate is 0% or 100% by construction, so "never + declined" is arithmetic and not evidence about the floor. Read the + refused record (`near_miss_samples`) BEFORE moving the dial — the last + time an arm sat here, every percentile said lower it and the refused + record showed the refusal was right. - `band_hugs_floor` — the weakest tenth of what an arm returns sits on its floor. The bar is doing the selecting and the score is not, so moving that floor changes how MUCH you get, not how good it is. diff --git a/src/scribe/services/retrieval_registry.py b/src/scribe/services/retrieval_registry.py index 5da53d7..fa4d527 100644 --- a/src/scribe/services/retrieval_registry.py +++ b/src/scribe/services/retrieval_registry.py @@ -86,6 +86,29 @@ class Point: quiet_because: str = "" + fixed_query: bool = False + """Whether this arm always searches the SAME query string. + + THE #3497 GUARD, ONE STEP OVER. `logs_unconditionally` below exists + because a warning computed over a LOGGING property read as a ranking + problem. This field exists because a warning computed over a QUERY-SHAPE + property does the same thing. + + An arm with a fixed query scores against one constant. Its decline rate is + therefore 0% or 100% and nothing in between — which of the two depends + only on whether the bar sits below or above that single number. So "never + returned nothing" says nothing at all about whether a floor is applied, + and `cannot_decline` — whose whole remedy is "check that it applies its + floor" — is uninformative here and skips these arms. + + What IS informative for them is the mirror image, and `reply_preferences` + names it in its own docstring: every call returning nothing means the bar + sits above the constant, no traffic will ever move it, and the arm is + dead. That has happened — 69 consecutive declines at 0.0006 under the bar + (see `retrieval_surfaces`) — so it gets its own warning rather than + inheriting one written for arms whose score can vary. + """ + logs_unconditionally: bool = True """Whether this arm writes a row even when it returns NOTHING. @@ -114,8 +137,13 @@ POINTS: dict[str, Point] = dict([ "the one line reserved for a preference at the prompt boundary"), _p("reuse_slot", UNBIDDEN, "the one line reserved for a reusable snippet"), _p("lesson_slot", UNBIDDEN, "the one line reserved for a lesson"), + # `fixed_query`: COMPLETION_QUERY is a module constant in + # services/reply_preferences.py, so this arm's top score is the same number + # on every call — measured at 0.791 across 45 consecutive calls, with p10, + # p50, p90, min and max all identical. Five equal percentiles is the tell. _p("report_preference", UNBIDDEN, - "the fixed question asked when a task finishes: how should this report read"), + "the fixed question asked when a task finishes: how should this report read", + fixed_query=True), # ── Asked: a caller wanted a ranked list ───────────────────────────── _p("mcp_search", ASKED, "an agent called search"), diff --git a/src/scribe/services/retrieval_telemetry.py b/src/scribe/services/retrieval_telemetry.py index f8bd4a4..6c1a477 100644 --- a/src/scribe/services/retrieval_telemetry.py +++ b/src/scribe/services/retrieval_telemetry.py @@ -556,12 +556,22 @@ def _compute_warnings(sources: dict, usage: dict, rule_usage: dict, # arms once recorded only their hits, so their decline count was # structurally zero and this warning would have fired on a LOGGING # defect while pointing the reader at the threshold. + # + # FOUR NOW. A FIXED-QUERY arm is exempt for the same reason one step + # over (#4232): it scores against one constant, so its decline rate is + # 0% or 100% and never in between, and which one it is depends only on + # where the bar sits relative to that single number. "Never returned + # nothing" is then not evidence about the floor — it is arithmetic — + # and this warning's own remedy, "check that it applies its floor", + # cannot be answered from it. Those arms get `fixed_query_never_clears` + # below, which asks the question that IS answerable for them. if ( calls >= min_calls and (b.get("zero_result_calls") or 0) == 0 and point is not None and point.kind == UNBIDDEN and point.logs_unconditionally + and not point.fixed_query ): out.append(_warn( "cannot_decline", @@ -572,6 +582,46 @@ def _compute_warnings(sources: dict, usage: dict, rule_usage: dict, source=name, calls=calls, zero_result_calls=0, )) + # ── A fixed-query arm that never clears its bar ────────────────── + # + # The mirror image of `cannot_decline`, and the state that actually + # threatens these arms. `reply_preferences` names it in its own + # docstring: "a fixed query makes this arm's score a constant and a + # floor a hair above it produces a dead arm no amount of traffic will + # ever reveal." + # + # For an arm whose score can vary, a window of all-empty calls is + # ordinary — it means nothing matched, which is an answer. For one + # whose score is a constant it means the bar is above that constant, + # and no volume of further calls will ever produce a different result. + # The arm is not quiet; it is switched off, and nothing else in this + # readout would say so. + # + # It has happened: `report_preference` logged 69 consecutive declines + # at 0.0006 under the bar. Note what that incident also proves — the + # fix is NOT automatically to lower the floor. Reading the refused + # record showed the refusal was correct, so this warning sends the + # reader to `near_miss_samples` rather than to the dial. + if ( + calls >= min_calls + and point is not None + and point.fixed_query + and point.logs_unconditionally + and (b.get("zero_result_calls") or 0) == calls + ): + out.append(_warn( + "fixed_query_never_clears", + f"{calls} calls, every one of them empty — and this arm always " + f"searches the same query, so its score is a constant. That " + f"means the bar sits above it and no amount of further traffic " + f"will change the result: the arm is off, not quiet. Read the " + f"record it refused (`near_miss_samples`) before touching the " + f"floor — the last time this arm sat here, every percentile " + f"said lower it and the refused record showed the refusal was " + f"right.", + source=name, calls=calls, zero_result_calls=calls, + )) + # ── Band hugs its floor ────────────────────────────────────────── # # Read on p10, the WEAKEST tenth of what the arm returned. If even diff --git a/tests/test_retrieval_warnings.py b/tests/test_retrieval_warnings.py index 8e3e0ac..c64119c 100644 --- a/tests/test_retrieval_warnings.py +++ b/tests/test_retrieval_warnings.py @@ -446,3 +446,80 @@ def test_a_quiet_arm_is_not_suspended_either_way() -> None: ws = warn({"auto_inject": src(calls=1, zero_result_calls=0, p10=0.705)}, floors={"auto_inject": 0.70}, floor_moves={"auto_inject": MOVED}) assert "floor_moved_mid_window" not in codes(ws) + + +# ── a fixed-query arm: decline rate is arithmetic, not evidence (#4232) ───── +# +# `report_preference` searches one constant string (COMPLETION_QUERY), so it +# scores against one number on every call. Its decline rate is therefore 0% or +# 100% and never in between, and which one depends only on where the bar sits +# relative to that constant. +# +# So the two warnings swap roles for these arms. "Never declined" stops being +# evidence about the floor — `cannot_decline`'s own remedy, "check that it +# applies its floor", is unanswerable from it. "Always declined" starts being +# evidence, because for a constant score it means the bar is above it and no +# further traffic will ever say otherwise. + + +def test_cannot_decline_is_silent_on_a_fixed_query_arm(): + """The live readout fired this on `report_preference` at 45 calls, 0 + empty, with p10 = p50 = p90 = min = max = 0.791 — five identical + percentiles, which is one record at one score rather than a ranking.""" + ws = warn({"report_preference": src(calls=45, zero_result_calls=0, p10=0.791)}) + assert "cannot_decline" not in codes(ws, "report_preference") + + +def test_cannot_decline_still_fires_where_the_rate_means_something(): + """The falsifier for the case above (rule 167). If this passes only + because the check was disabled rather than narrowed, this fails.""" + ws = warn({"auto_inject": src(calls=N, zero_result_calls=0)}) + assert "cannot_decline" in codes(ws, "auto_inject") + + +def test_a_fixed_query_arm_that_never_clears_its_bar_is_named(): + """69 consecutive declines at 0.0006 under the bar is a state this arm has + actually been in. Nothing else in the readout would have said so: it looks + exactly like an arm with nothing to report.""" + ws = warn({"report_preference": src(calls=45, zero_result_calls=45)}) + assert "fixed_query_never_clears" in codes(ws, "report_preference") + + detail = next(w["detail"] for w in ws if w["code"] == "fixed_query_never_clears") + assert "near_miss_samples" in detail, ( + "the last time this fired, every percentile said lower the floor and " + "the refused record showed the refusal was right — so the warning has " + "to send the reader to the record, not to the dial" + ) + + +def test_an_ordinary_arm_returning_nothing_all_window_is_not_dead(): + """For an arm whose score can vary, an empty window means nothing matched, + which is an answer rather than a fault.""" + ws = warn({"auto_inject": src(calls=N, zero_result_calls=N)}) + assert "fixed_query_never_clears" not in codes(ws, "auto_inject") + + +def test_a_fixed_query_arm_that_sometimes_clears_is_not_dead(): + """Only ALL-empty says the bar is above the constant. Anything in between + means the score is not actually constant, and the premise is wrong.""" + ws = warn({"report_preference": src(calls=45, zero_result_calls=44)}) + assert "fixed_query_never_clears" not in codes(ws, "report_preference") + + +def test_the_dead_arm_warning_still_needs_volume(): + ws = warn({"report_preference": src(calls=N - 1, zero_result_calls=N - 1)}) + assert "fixed_query_never_clears" not in codes(ws) + + +def test_the_registry_declares_which_arms_ask_a_fixed_question(): + """Asserted on structure (rule 167), and able to fail: if `fixed_query` + is dropped or defaults to True, one of these two halves breaks.""" + from scribe.services.retrieval_registry import POINTS + + assert POINTS["report_preference"].fixed_query is True, ( + "services/reply_preferences.py::COMPLETION_QUERY is a module constant" + ) + # An arm whose query is built from the prompt, the file or the command is + # not fixed, and marking one would silence a warning that works there. + for varying in ("auto_inject", "write_path", "pre_tool_rule", "prompt_rule"): + assert POINTS[varying].fixed_query is False, varying -- 2.54.0 From 4f2977b848bb2cff90febec7bdc34b08e44d1579 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 08:45:43 -0400 Subject: [PATCH 2/9] fix(tasks): add_task_log wrote to a surface no agent could read back (#4241) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The work log reached the web UI through routes/task_logs.py and nothing else. get_task returned only the body — a claim written once, before the work — with the record written during it invisible beside it. So a stale body arrived with nothing to contradict it, and this session rebuilt work that had already shipped, with the evidence sitting in the task's own logs. Read side, scoped through the access layer (rule 78): - logs_for_task / count_logs_for_task / log_counts_for_tasks in services/task_logs.py. Scoped by who may read the TASK rather than by who wrote the entry: list_logs filters TaskLog.user_id == user_id, which hands a shared collaborator an empty list reading as "no work has been done". The page query folds readable_notes_clause into the same statement so the permission does not become an N+1. - get_task returns work_log; list_tasks and get_milestone steps carry log_count, zero-filled so "none" is a count and not a missing key. Elision keeps both ends. The newest entry arrives whole to 4000 chars because it answers "where does this stand"; older ones are shortened from the MIDDLE, never the head. A head cut selects what a reader sees by character position, which is uncorrelated with what matters — an entry closing with "so this shipped in 04775c3" loses the one sentence that answers the question, and a truncated flag says something went, never whether it mattered. The gap states how many characters it covers. conftest gains an autouse stub for the new read arm, same reasoning as _no_rule_arm: three widely-called tools grew a database read, and the existing call sites should not each have to learn about it. Raised while reviewing this: search() has the same shape and worse — body[:240] with no marker at all, while the chunk that actually matched sits unused in the row that won. Filed as #4243, not fixed here. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/mcp/tools/milestones.py | 16 +- src/scribe/mcp/tools/tasks.py | 154 +++++++++++- src/scribe/services/notes.py | 17 +- src/scribe/services/task_logs.py | 78 +++++- tests/conftest.py | 26 ++ tests/test_task_work_log_surface.py | 375 ++++++++++++++++++++++++++++ 6 files changed, 660 insertions(+), 6 deletions(-) create mode 100644 tests/test_task_work_log_surface.py diff --git a/src/scribe/mcp/tools/milestones.py b/src/scribe/mcp/tools/milestones.py index 9e4b6f9..fec165e 100644 --- a/src/scribe/mcp/tools/milestones.py +++ b/src/scribe/mcp/tools/milestones.py @@ -15,6 +15,7 @@ from scribe.mcp._context import current_user_id from scribe.services import dedup as dedup_svc from scribe.services import milestones as milestones_svc from scribe.services import notes as notes_svc +from scribe.services import task_logs as task_logs_svc from scribe.services import rulebooks as rulebooks_svc from scribe.services import trash as trash_svc from scribe.services.record_refs import refuse_guessed_ids @@ -53,6 +54,11 @@ async def get_milestone(milestone_id: int) -> dict: what it is and where it stands, not its whole body — read a step in full with get_task(id). A plan with forty long steps is otherwise too big to arrive inline, and the design is in the milestone body. + + Each step also carries `log_count`. A step's STATUS is set by hand and a + plan is exactly where that goes stale; the count says which steps have a + work log that would say otherwise, so `get_task(id)` goes to the step + with a record rather than to each one in turn (#4241). """ uid = current_user_id() milestone = await milestones_svc.get_milestone(uid, milestone_id) @@ -65,11 +71,19 @@ async def get_milestone(milestone_id: int) -> dict: applicable = await rulebooks_svc.get_applicable_rules( project_id=milestone.project_id, user_id=uid, ) + log_counts = await task_logs_svc.log_counts_for_tasks( + uid, [int(t.id) for t in steps] + ) out = milestone.to_dict() out.update(progress) return { "milestone": out, - "steps": [notes_svc.brief_row(t, {milestone.id: milestone.title}) for t in steps], + "steps": [ + notes_svc.brief_row( + t, {milestone.id: milestone.title}, log_counts=log_counts + ) + for t in steps + ], **rulebooks_svc.rules_payload(applicable, user_id=uid, source="get_milestone"), } diff --git a/src/scribe/mcp/tools/tasks.py b/src/scribe/mcp/tools/tasks.py index 24a740f..9562d38 100644 --- a/src/scribe/mcp/tools/tasks.py +++ b/src/scribe/mcp/tools/tasks.py @@ -3,7 +3,13 @@ Tasks are notes with a non-null `status` — same model, different filter. Wrappers call services/notes.py for CRUD with is_task=True and add the task-specific fields (status, priority, due_date, parent_id), plus -services/task_logs.py for add_task_log. +services/task_logs.py for add_task_log AND for reading those logs back — +`get_task` returns them and `list_tasks` counts them. For most of this +module's life `add_task_log` wrote to a surface no agent could read: the +entries reached the web UI and nothing else, so a session opening a task +saw only the body — a claim written before the work — with the record +written during it invisible beside it. A stale body then had nothing to +contradict it, and shipped work got rebuilt (#4241). There is no delete_task — matches the existing fable-mcp surface. Cancel by updating status to "cancelled". @@ -41,6 +47,92 @@ from scribe.services.note_usage import record_pulled from scribe.services.record_refs import refuse_guessed_ids +# A work log entry is prose, often long — the discipline asks for what was +# decided and why, not a line of status. Two caps, not one, because the entries +# are not equally useful: the NEWEST answers "where does this actually stand", +# which is the question the block exists for, so it arrives whole up to a +# generous ceiling. Older entries are there to say what happened and when, and +# a headline does that. +_WORK_LOG_ENTRIES = 3 +_WORK_LOG_CHARS = 800 +_WORK_LOG_LATEST_CHARS = 4000 + +_WORK_LOG_ADVICE = ( + "The body is a CLAIM, written once before the work. These entries are the " + "RECORD, written during and after it. Where the two disagree the log is " + "later — read it before acting on what the body says the status is." +) + + +def elide(text: str, budget: int) -> tuple[str, bool]: + """Cut to `budget` characters from the MIDDLE, keeping both ends. + + A head-only cut — `text[:800]` — decides what a reader sees by character + position, which is uncorrelated with what matters. Prose does not put its + conclusion first: a log entry that opens with what was tried and closes + with "so this shipped in 04775c3" loses exactly the sentence that answers + the question, and the reader cannot tell, because a truncation marker says + that something was removed and never whether it mattered. + + So keep the opening (what this entry is about) AND the closing (where it + landed), and say in between how much went. Two thirds to the head because + that is where the subject is established; the tail needs less to carry a + conclusion. `budget` of 0 means no cut. + """ + if budget <= 0 or len(text) <= budget: + return text, False + head_len = max(1, budget * 2 // 3) + tail_len = max(1, budget - head_len) + omitted = len(text) - head_len - tail_len + head = text[:head_len].rstrip() + tail = text[-tail_len:].lstrip() + return f"{head}\n\n[… {omitted} characters omitted …]\n\n{tail}", True + + +def work_log_payload( + logs: list, total: int, chars: int, latest_chars: int = _WORK_LOG_LATEST_CHARS +) -> dict: + """The `work_log` block: recent entries newest-first, plus what was elided. + + `total` is the count of ALL entries, not of `logs` — a reader has to be + able to tell "this task has no record" from "you were shown the last three + of nine", and those are the same response if the count comes from the + entries handed over. + """ + entries = [] + for i, log in enumerate(logs): + row = log.to_dict() if hasattr(log, "to_dict") else dict(log) + row.pop("updated_at", None) + content = row.get("content") or "" + # The first row IS the newest — logs_for_task orders descending. + budget = latest_chars if i == 0 else chars + if chars <= 0: + budget = 0 + text, cut = elide(content, budget) + row["content"] = text + if cut: + row["full_length"] = len(content) + row["truncated"] = True + entries.append(row) + + out: dict = {"total": total, "entries": entries} + if total == 0: + return out + + out["advice"] = _WORK_LOG_ADVICE + not_shown = total - len(entries) + if not_shown > 0: + out["not_shown"] = not_shown + if not_shown > 0 or any(e.get("truncated") for e in entries): + out["read_all"] = ( + "Entries were shortened or omitted. " + "get_task(task_id, log_limit=0, log_chars=0) returns every entry " + "in full — reach for it rather than judging from what is here, " + "which was selected by recency and length, not by relevance." + ) + return out + + async def list_tasks( limit: int = 20, offset: int = 0, @@ -75,10 +167,27 @@ async def list_tasks( offset=max(0, offset), ) titles = await milestones_svc.titles_for({n.milestone_id for n in rows}) - return {"tasks": [notes_svc.brief_row(n, titles) for n in rows], "total": total} + # One aggregate for the page, zero-filled: a reader scanning a list needs + # to know WHICH rows carry a record before choosing what to open, and a + # missing key would read as "no logs" on every row rather than on the + # rows that have none. + log_counts = await task_logs_svc.log_counts_for_tasks( + uid, [int(n.id) for n in rows] + ) + return { + "tasks": [ + notes_svc.brief_row(n, titles, log_counts=log_counts) for n in rows + ], + "total": total, + } -async def get_task(task_id: int, project_id: int = 0) -> dict: +async def get_task( + task_id: int, + project_id: int = 0, + log_limit: int = _WORK_LOG_ENTRIES, + log_chars: int = _WORK_LOG_CHARS, +) -> dict: """Fetch a single Scribe task by ID. Returns id, title, body, status, priority, tags, project_id, milestone_id, @@ -89,6 +198,27 @@ async def get_task(task_id: int, project_id: int = 0) -> dict: kind=plan tasks, the response also includes the project's applicable_rules and project_rules (new plans are milestones — use get_milestone for those). + AND `work_log` — the entries add_task_log wrote, newest first, with + `total` for how many exist. READ IT BEFORE TRUSTING THE BODY. A body is + written once, at the start, when the least is known; the log is written + during the work and after it. A task whose body says "not started" and + whose log records a partial ship is not a contradiction to resolve — the + log is simply later. This block exists because its absence cost a session + a day of rebuilding work that had already shipped (#4241). + + The NEWEST entry arrives whole (to 4000 characters); older ones are + shortened to a headline. Anything shortened is cut from the MIDDLE, so + the opening and the closing both survive — a log entry's conclusion is at + its end — and the gap states how many characters went. An entry that was + cut says so and carries its `full_length`, and the block as a whole says + when it is showing you less than the record holds. + + Args: + log_limit: How many of the most recent entries to include (default 3). + 0 returns every entry. + log_chars: Budget for the OLDER entries (default 800). 0 returns + every entry in full, the newest included. + A task another user shared with you also carries `shared`, `owner` and `permission` — it's their work item, not one you took on. @@ -127,6 +257,17 @@ async def get_task(task_id: int, project_id: int = 0) -> dict: await systems_tools.attach_systems( uid, getattr(note, "user_id", uid) or uid, data, note.id, note.project_id ) + # Counted separately rather than inferred from the rows handed back: with a + # limit applied, len(entries) is the size of the window, not of the record, + # and "3 entries" and "the last 3 of 9" have to read differently. + log_rows = await task_logs_svc.logs_for_task( + uid, int(note.id), limit=max(0, log_limit) + ) + log_total = ( + len(log_rows) if log_limit <= 0 + else await task_logs_svc.count_logs_for_task(uid, int(note.id)) + ) + data["work_log"] = work_log_payload(log_rows, log_total, max(0, log_chars)) record_pulled( user_id=uid, note_id=int(note.id), source="mcp_get_task", project_id=project_id, @@ -353,6 +494,13 @@ async def add_task_log(task_id: int, content: str) -> dict: without overwriting the task's main body. Each entry is stored separately and shown chronologically in the task view. + What you write here comes back from `get_task` as `work_log`, newest + first, and is counted on every row of `list_tasks` and every step of + `get_milestone` — so write for the session that opens this task next, not + for a reader who already knows what you were doing. What that reader + cannot get from the body or the diff is what you tried, what you ruled + out, and where it actually stands. + The response shows the task's `systems` — or, if the task is an untagged project record, the `systems_hint` question: logging work IS working in some area, so answer it (update_task with system_ids, or create_system diff --git a/src/scribe/services/notes.py b/src/scribe/services/notes.py index 7a21123..165eaee 100644 --- a/src/scribe/services/notes.py +++ b/src/scribe/services/notes.py @@ -1096,7 +1096,20 @@ async def get_note_for_user( # A field most rows leave empty (a one-line description, a parent, a due date) # is attached only when set: a hundred rows of `null` are a hundred chances to # learn to skip the key (#2483), and the bytes are the thing being cut. -def brief_row(note: Note, milestone_titles: dict[int, str] | None = None) -> dict: +def brief_row( + note: Note, + milestone_titles: dict[int, str] | None = None, + log_counts: dict[int, int] | None = None, +) -> dict: + """One task as a list row — what it is and where it stands, not its body. + + `log_counts` is a whole page's work-log counts fetched in one aggregate + (task_logs.log_counts_for_tasks). Passed in rather than looked up here so + the N+1 stays impossible, and applied to tasks only: a note has no work + log. Zero-filled when the mapping is given, because a row that omits the + key says "no record" on every row rather than on the ones with none, and + knowing WHICH rows carry a record is how a reader decides what to open. + """ row = { "id": note.id, "title": note.title, @@ -1120,4 +1133,6 @@ def brief_row(note: Note, milestone_titles: dict[int, str] | None = None) -> dic row["parent_id"] = note.parent_id if note.due_date: row["due_date"] = iso(note.due_date) + if log_counts is not None: + row["log_count"] = log_counts.get(int(note.id), 0) return row diff --git a/src/scribe/services/task_logs.py b/src/scribe/services/task_logs.py index 7a96165..e06975f 100644 --- a/src/scribe/services/task_logs.py +++ b/src/scribe/services/task_logs.py @@ -2,11 +2,12 @@ import logging from datetime import datetime, timezone -from sqlalchemy import select +from sqlalchemy import func, select from scribe.models import async_session from scribe.models.task_log import TaskLog from scribe.models.note import Note +from scribe.services.access import can_read_note, readable_notes_clause logger = logging.getLogger(__name__) @@ -48,6 +49,81 @@ async def list_logs(user_id: int, task_id: int) -> list[TaskLog]: return list(result.scalars().all()) +async def logs_for_task( + user_id: int, task_id: int, limit: int = 0 +) -> list[TaskLog]: + """Every work log on a task, NEWEST FIRST, whoever wrote it. + + Two differences from `list_logs`, both deliberate: + + 1. Scoped by who may read the TASK, not by who wrote each entry. A work + log belongs to the task, and on a shared task the owner's record is + exactly what a collaborator needs — `list_logs`' `TaskLog.user_id == + user_id` returns them an empty list, which reads as "no work has been + done" rather than "not yours". The permission question is asked of the + note through `can_read_note` (rule #78), so this is not an unscoped + read with the check left to the caller. + 2. Newest first, because the question a reader asks of a work log is + "where does this actually stand", and the answer is the last entry. + `list_logs` stays ascending — the web UI renders a narrative. + + `limit` of 0 means all of them. An unreadable or missing task returns [] + the same way an empty one does: a work log is not a channel for proving a + record exists. + """ + if not await can_read_note(user_id, task_id): + return [] + async with async_session() as session: + stmt = ( + select(TaskLog) + .where(TaskLog.task_id == task_id) + .order_by(TaskLog.created_at.desc(), TaskLog.id.desc()) + ) + if limit > 0: + stmt = stmt.limit(limit) + result = await session.execute(stmt) + return list(result.scalars().all()) + + +async def count_logs_for_task(user_id: int, task_id: int) -> int: + """How many work logs a task carries. Same scoping as logs_for_task.""" + if not await can_read_note(user_id, task_id): + return 0 + async with async_session() as session: + result = await session.execute( + select(func.count(TaskLog.id)).where(TaskLog.task_id == task_id) + ) + return int(result.scalar() or 0) + + +async def log_counts_for_tasks( + user_id: int, task_ids: list[int] +) -> dict[int, int]: + """Log counts for a whole page of tasks in ONE query. + + A per-row lookup would be N+1 by construction — and so would a per-row + `can_read_note` — which is the reason a list surface would otherwise keep + omitting this and leave a reader to guess which rows carry a record. So + the permission is expressed as set membership with + `readable_notes_clause` and folded into the same statement, the pattern + that function exists for. + + Tasks with no logs are absent from the mapping; callers zero-fill, so + "none" reads as a count rather than a missing key. + """ + ids = [int(t) for t in task_ids if t] + if not ids: + return {} + async with async_session() as session: + result = await session.execute( + select(TaskLog.task_id, func.count(TaskLog.id)) + .join(Note, Note.id == TaskLog.task_id) + .where(TaskLog.task_id.in_(ids), readable_notes_clause(user_id)) + .group_by(TaskLog.task_id) + ) + return {int(tid): int(n) for tid, n in result.all()} + + async def update_log( user_id: int, log_id: int, diff --git a/tests/conftest.py b/tests/conftest.py index d562c8f..24db63d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -102,6 +102,32 @@ def _no_supersession(): yield +@pytest.fixture(autouse=True) +def _no_task_log_arm(): + """Stub the task-log read arm that get_task / list_tasks / get_milestone + grew in #4241. + + Autouse for the reason _no_rule_arm is: those three tools now read work + logs, and the reads go through the access layer to Postgres. Every unit + test that opens a task — and most of them do, because a task is what this + codebase is mostly about — would otherwise try to reach the fake + DATABASE_URL this file sets, to learn that a fake task has no logs. + + The arm's own behaviour is covered where it belongs: the payload shape and + the tool wiring in tests/test_task_work_log_surface.py, which re-patches + these explicitly, and the ACL scoping against real Postgres in + tests/test_integration_task_work_log.py. A test that wants the arm live + re-patches it, same as the rules arm. + """ + with patch("scribe.services.task_logs.logs_for_task", + AsyncMock(return_value=[])), \ + patch("scribe.services.task_logs.count_logs_for_task", + AsyncMock(return_value=0)), \ + patch("scribe.services.task_logs.log_counts_for_tasks", + AsyncMock(return_value={})): + yield + + @pytest.fixture(autouse=True) def _no_rule_arm(): """Stub the write-path hint's standing-RULES arm (milestone 307). diff --git a/tests/test_task_work_log_surface.py b/tests/test_task_work_log_surface.py new file mode 100644 index 0000000..9376173 --- /dev/null +++ b/tests/test_task_work_log_surface.py @@ -0,0 +1,375 @@ +"""The work log is READABLE from the tools an agent opens a task with (#4241). + +`add_task_log` wrote to a surface no agent could read back: the entries +reached the web UI through routes/task_logs.py and nothing else. So a session +opening a task saw only the body — a claim written once, before the work — +with the record written during it invisible beside it. A stale body had +nothing to contradict it, and shipped work got rebuilt. + +These pin the read, not the write. The payload shape is tested against the +real `work_log_payload`; the three tools are tested by re-patching the arm +that tests/conftest.py stubs autouse, so each one is exercised live here and +nowhere else. +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.mcp.tools.milestones import get_milestone +from scribe.mcp.tools.tasks import ( + elide, get_task, list_tasks, work_log_payload, +) +from scribe.services.notes import brief_row +from tests.helpers import fake_task, make_mock_session + +pytestmark = pytest.mark.usefixtures("_bind_user") + + +def fake_log(log_id: int = 1, content: str = "did the thing", **attrs): + """A stand-in TaskLog. `to_dict` is what the payload builder consumes.""" + row = { + "id": log_id, + "task_id": 1, + "user_id": 7, + "content": content, + "duration_minutes": None, + "created_at": "2026-09-21T12:00:00+00:00", + "updated_at": "2026-09-21T12:00:00+00:00", + } + row.update(attrs) + log = MagicMock() + log.to_dict = MagicMock(return_value=row) + log.content = row["content"] + log.id = row["id"] + return log + + +def _live_arm(logs=(), total=0, counts=None): + """Re-patch conftest's autouse stub so the arm under test is live.""" + return ( + patch("scribe.services.task_logs.logs_for_task", + AsyncMock(return_value=list(logs))), + patch("scribe.services.task_logs.count_logs_for_task", + AsyncMock(return_value=total)), + patch("scribe.services.task_logs.log_counts_for_tasks", + AsyncMock(return_value=counts or {})), + ) + + +# --------------------------------------------------------------------------- +# The payload shape +# --------------------------------------------------------------------------- + +def test_no_logs_reads_as_a_count_not_a_missing_key(): + """"This task has no record" and "you were not shown the record" are + different answers, and a caller has to be able to tell them apart.""" + out = work_log_payload([], 0, 800) + assert out == {"total": 0, "entries": []} + # No advice on an empty log: there is no body/record disagreement to warn + # about, and a standing paragraph on every task is noise. + assert "advice" not in out + + +def test_entries_carry_the_advice_that_the_log_outranks_the_body(): + out = work_log_payload([fake_log()], 1, 800) + assert "advice" in out + advice = out["advice"].lower() + assert "claim" in advice and "record" in advice + # The operative instruction: which one to believe when they disagree. + assert "later" in advice + + +def test_elision_keeps_the_end_where_the_conclusion_lives(): + """The whole reason this is not `text[:n]`. + + Prose does not put its conclusion first. An entry that opens with what was + attempted and closes with "so this shipped in 04775c3" loses the one + sentence that answers the question if the cut is taken from the head — and + a `truncated: true` flag tells a reader that something went, never whether + it mattered. Both ends survive, and the gap says how much is missing. + """ + text = "Tried the form column. " + ("m" * 2000) + " CONCLUSION: shipped in 04775c3." + out, cut = elide(text, 300) + assert cut is True + assert out.startswith("Tried the form column.") + assert out.rstrip().endswith("shipped in 04775c3.") + assert "characters omitted" in out + + +def test_elision_states_how_much_went(): + out, _ = elide("a" * 1000, 100) + assert "900 characters omitted" in out + + +def test_short_text_is_returned_untouched(): + out, cut = elide("brief", 300) + assert out == "brief" + assert cut is False + + +def test_the_newest_entry_arrives_whole_and_older_ones_are_headlines(): + """The newest entry answers "where does this stand", which is the question + the block exists for — so it is not competing for budget with history.""" + newest = fake_log(9, content="N" * 3000) + older = fake_log(8, content="O" * 3000) + out = work_log_payload([newest, older], 2, chars=800, latest_chars=4000) + assert out["entries"][0]["content"] == "N" * 3000 + assert "truncated" not in out["entries"][0] + assert out["entries"][1]["truncated"] is True + assert out["entries"][1]["full_length"] == 3000 + + +def test_even_the_newest_entry_is_capped_somewhere(): + out = work_log_payload([fake_log(content="x" * 9000)], 1, chars=800, + latest_chars=4000) + entry = out["entries"][0] + assert entry["truncated"] is True + assert entry["full_length"] == 9000 + assert "read_all" in out + + +def test_read_all_says_the_window_was_chosen_by_recency_not_relevance(): + """A pointer to the fuller read is only useful if the reader knows the + shown part was not selected for being the pertinent part.""" + out = work_log_payload([fake_log(content="x" * 9000)], 1, chars=800, + latest_chars=4000) + assert "relevance" in out["read_all"] + + +def test_total_is_the_record_and_entries_are_the_window(): + """The bug this guards: reporting len(entries) as the total, so "the last + three of nine" is indistinguishable from "three".""" + out = work_log_payload([fake_log(1), fake_log(2), fake_log(3)], 9, 800) + assert out["total"] == 9 + assert len(out["entries"]) == 3 + assert out["not_shown"] == 6 + assert "get_task" in out["read_all"] + + +def test_an_untruncated_full_window_advertises_nothing_further(): + out = work_log_payload([fake_log(1), fake_log(2)], 2, 800) + assert "not_shown" not in out + assert "read_all" not in out + + +# --------------------------------------------------------------------------- +# get_task — the tool that failed +# --------------------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_get_task_returns_the_work_log(): + fake = fake_task(id=4208, title="stale body", parent_id=None) + p1, p2, p3 = _live_arm(logs=[fake_log(content="partially shipped in 04775c3")], + total=1) + with patch("scribe.mcp.tools.tasks.notes_svc.get_note_for_user", + AsyncMock(return_value=(fake, "owner"))), p1, p2, p3: + out = await get_task(task_id=4208) + assert "work_log" in out + assert out["work_log"]["total"] == 1 + assert "04775c3" in out["work_log"]["entries"][0]["content"] + + +@pytest.mark.asyncio +async def test_get_task_says_a_task_has_no_log_rather_than_omitting_the_key(): + fake = fake_task(id=1, parent_id=None) + p1, p2, p3 = _live_arm(logs=[], total=0) + with patch("scribe.mcp.tools.tasks.notes_svc.get_note_for_user", + AsyncMock(return_value=(fake, "owner"))), p1, p2, p3: + out = await get_task(task_id=1) + assert out["work_log"] == {"total": 0, "entries": []} + + +@pytest.mark.asyncio +async def test_get_task_counts_every_entry_not_just_the_window(): + fake = fake_task(id=1, parent_id=None) + window = [fake_log(i) for i in (9, 8, 7)] + p1, p2, p3 = _live_arm(logs=window, total=9) + with patch("scribe.mcp.tools.tasks.notes_svc.get_note_for_user", + AsyncMock(return_value=(fake, "owner"))), p1, p2, p3: + out = await get_task(task_id=1) + assert out["work_log"]["total"] == 9 + assert out["work_log"]["not_shown"] == 6 + + +@pytest.mark.asyncio +async def test_get_task_default_window_is_three_entries(): + fake = fake_task(id=1, parent_id=None) + limit_seen = {} + + async def _capture_limit(uid, task_id, limit=0): + limit_seen["limit"] = limit + return [] + + with patch("scribe.mcp.tools.tasks.notes_svc.get_note_for_user", + AsyncMock(return_value=(fake, "owner"))), \ + patch("scribe.services.task_logs.logs_for_task", _capture_limit), \ + patch("scribe.services.task_logs.count_logs_for_task", + AsyncMock(return_value=0)): + await get_task(task_id=1) + assert limit_seen["limit"] == 3 + + +@pytest.mark.asyncio +async def test_log_limit_zero_asks_for_every_entry_and_skips_the_count_query(): + """0 means "all" here because it means "no cap" in the service it calls. + With no cap the rows ARE the total, so a second query would be waste.""" + fake = fake_task(id=1, parent_id=None) + counter = AsyncMock(return_value=99) + p1, _, p3 = _live_arm(logs=[fake_log(1), fake_log(2)]) + with patch("scribe.mcp.tools.tasks.notes_svc.get_note_for_user", + AsyncMock(return_value=(fake, "owner"))), \ + p1, p3, patch("scribe.services.task_logs.count_logs_for_task", counter): + out = await get_task(task_id=1, log_limit=0) + assert out["work_log"]["total"] == 2 + assert counter.await_count == 0 + + +@pytest.mark.asyncio +async def test_get_task_docstring_tells_a_reader_the_log_outranks_the_body(): + """#2846: the docstring IS the agent-facing contract. A `work_log` key + that nothing tells the reader to prefer over a stale body is the same + failure one layer up.""" + doc = (get_task.__doc__ or "").lower() + assert "work_log" in doc + assert "body" in doc + + +# --------------------------------------------------------------------------- +# The list surfaces — which rows carry a record +# --------------------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_list_tasks_rows_carry_a_zero_filled_log_count(): + rows = [fake_task(id=1, milestone_id=None), fake_task(id=2, milestone_id=None)] + _, _, p3 = _live_arm(counts={1: 4}) + with patch("scribe.mcp.tools.tasks.notes_svc.list_notes", + AsyncMock(return_value=(rows, 2))), p3: + out = await list_tasks() + assert out["tasks"][0]["log_count"] == 4 + # Zero-filled, not omitted: a missing key would read as "no logs" on every + # row rather than on the rows that have none. + assert out["tasks"][1]["log_count"] == 0 + + +@pytest.mark.asyncio +async def test_list_tasks_counts_the_page_in_one_query(): + rows = [fake_task(id=i, milestone_id=None) for i in range(1, 6)] + counter = AsyncMock(return_value={}) + with patch("scribe.mcp.tools.tasks.notes_svc.list_notes", + AsyncMock(return_value=(rows, 5))), \ + patch("scribe.services.task_logs.log_counts_for_tasks", counter): + await list_tasks() + assert counter.await_count == 1 + assert sorted(counter.await_args.args[1]) == [1, 2, 3, 4, 5] + + +@pytest.mark.asyncio +async def test_get_milestone_steps_carry_a_log_count(): + """A step's status is set by hand, and a plan is exactly where that goes + stale — so a plan reader needs to see which steps have a record.""" + milestone = MagicMock(id=385, title="Lessons", project_id=2) + milestone.to_dict = MagicMock(return_value={"id": 385, "title": "Lessons"}) + steps = [fake_task(id=3735, milestone_id=385), + fake_task(id=3736, milestone_id=385)] + counter = AsyncMock(return_value={3735: 2}) + with patch("scribe.mcp.tools.milestones.milestones_svc.get_milestone", + AsyncMock(return_value=milestone)), \ + patch("scribe.mcp.tools.milestones.milestones_svc.get_milestone_progress", + AsyncMock(return_value={})), \ + patch("scribe.mcp.tools.milestones.notes_svc.list_notes", + AsyncMock(return_value=(steps, 2))), \ + patch("scribe.mcp.tools.milestones.rulebooks_svc.get_applicable_rules", + AsyncMock(return_value=[])), \ + patch("scribe.mcp.tools.milestones.rulebooks_svc.rules_payload", + MagicMock(return_value={})), \ + patch("scribe.services.task_logs.log_counts_for_tasks", counter): + out = await get_milestone(milestone_id=385) + assert out["steps"][0]["log_count"] == 2 + assert out["steps"][1]["log_count"] == 0 + + +def test_brief_row_without_a_mapping_is_unchanged(): + """The other three brief_row callers (list_notes, list_system_records) + pass no counts and must not grow a misleading zero.""" + row = brief_row(fake_task(id=1, milestone_id=None, due_date=None)) + assert "log_count" not in row + + +# --------------------------------------------------------------------------- +# The service read — scoped by who may read the TASK (rule #78) +# --------------------------------------------------------------------------- + +@pytest.mark.asyncio +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=[]))) + )) + with patch("scribe.services.task_logs.can_read_note", + AsyncMock(return_value=True)), \ + patch("scribe.services.task_logs.async_session", + MagicMock(return_value=session)): + await svc.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 + + +@pytest.mark.asyncio +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) + 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 opened.call_count == 0 + + +@pytest.mark.asyncio +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]) + assert out == {1: 3} + stmt = str(session.execute.await_args.args[0]) + assert "notes" in stmt.lower() + + +@pytest.mark.asyncio +async def test_no_ids_asks_the_database_nothing(): + from scribe.services import task_logs as svc + + opened = MagicMock() + with patch("scribe.services.task_logs.async_session", opened): + assert await svc.log_counts_for_tasks(7, []) == {} + assert opened.call_count == 0 -- 2.54.0 From fdc07f2a2b5a0df07968069e7d6a028bc5a5435a Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 08:50:16 -0400 Subject: [PATCH 3/9] fix(search): show the passage that matched, not the opening of the body (#4243) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/mcp/tools/search.py | 67 +++++++++++++++++++++++++- src/scribe/mcp/tools/tasks.py | 26 +--------- src/scribe/services/embeddings.py | 39 +++++++++++++-- src/scribe/services/text.py | 38 +++++++++++++++ tests/test_mcp_tool_search.py | 75 +++++++++++++++++++++++++++-- tests/test_pull_telemetry.py | 23 ++++++++- tests/test_task_work_log_surface.py | 27 ++++++----- 7 files changed, 245 insertions(+), 50 deletions(-) create mode 100644 src/scribe/services/text.py diff --git a/src/scribe/mcp/tools/search.py b/src/scribe/mcp/tools/search.py index 4623ca9..027f397 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.text import elide from scribe.services.embeddings import ( DEFAULT_SIMILARITY_THRESHOLD, semantic_search_milestones, semantic_search_notes, semantic_search_rules, @@ -102,6 +103,55 @@ async def _search_milestones(uid: int, q: str, limit: int, project_id: int) -> d } +# A matched chunk is at most _CHUNK_CHAR_BUDGET (1400) characters, and it is +# the evidence the ranking was built on — so it is worth more room than the 240 +# characters of document opening this used to send. Elision inside a chunk is +# far less lossy than a head cut of a whole record: the region is already the +# right one. +_EXCERPT_CHARS = 1000 + + +def result_excerpt(note, chunk: dict | None) -> dict: + """The part of a record a caller judges "should I open this?" on. + + This used to be `(note.body or "")[:240]` — the document's opening, with + no marker that anything had been cut, 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 was semantic and per-chunk, and + `semantic_search_notes` collapses to best-chunk-per-note — so the system + already knows which passage earned the hit and used to discard it. A + record could rank first on its sixth paragraph and be previewed by its + first, which the search had already judged less relevant, and the caller + would decide from that and never know (#4243). + + So: show the matched passage when there is one, the body when it fits, and + in either case SAY which of the two this is. A caller that cannot tell an + excerpt from a whole record cannot tell whether looking deeper is worth it, + which is the only decision this field supports. + """ + body = note.body or "" + matched = (chunk or {}).get("text") or "" + out: dict = {"body_length": len(body)} + + if matched.strip(): + text, cut = elide(matched.strip(), _EXCERPT_CHARS) + out["excerpt"] = text + out["excerpt_is"] = "matched_passage" + if (chunk or {}).get("index") is not None: + out["chunk_index"] = int(chunk["index"]) + else: + # No stored chunk — an un-embedded record, or a caller that passed no + # report. Fall back to the opening, and name it as the opening rather + # than letting it pass for the relevant part. + text, cut = elide(body, _EXCERPT_CHARS) + out["excerpt"] = text + out["excerpt_is"] = "body_opening" + if cut or (out["excerpt_is"] == "matched_passage" and len(matched) < len(body)): + out["read_full"] = "get_note / get_task by id for the whole record." + return out + + async def search( q: str, content_type: str = "all", @@ -150,9 +200,21 @@ async def search( list_system_records gives the same slice unranked. Returns: - {"results": [{"id", "title", "body", "is_task", "tags", "similarity"}], + {"results": [{"id", "title", "excerpt", "excerpt_is", "body_length", + "is_task", "tags", "similarity"}], "total": int} + `excerpt` is a SPAN of the record, not the record. `excerpt_is` says + which span: "matched_passage" is the chunk that actually earned the + hit — the evidence the ranking was built on, and the right thing to + judge relevance from. "body_opening" is a fallback for a record with + no stored chunk, and is only the beginning of the text, which may say + nothing about why it matched. `body_length` is the whole record's + size, so a long record previewed by a short span is visible as one; + `read_full` appears when there is more, and opening the id by + get_note / get_task is how you get it. Judge from the passage, not + from the fact that a preview looked thin. + A result marked `shared: true` with an `owner` belongs to another user — that person's suggestion, not the operator's own record or settled practice. Weigh it on its merits and say whose it is when you use it. @@ -195,12 +257,13 @@ async def search( owners = await owner_names_for( {int(note.user_id) for _s, note in raw if note.user_id != uid} ) + chunks = report.get("best_chunk") or {} return { "results": [ { "id": note.id, "title": note.title, - "body": (note.body or "")[:240], + **result_excerpt(note, chunks.get(int(note.id))), "is_task": bool(note.is_task), "tags": list(note.tags or []), "similarity": float(score), diff --git a/src/scribe/mcp/tools/tasks.py b/src/scribe/mcp/tools/tasks.py index 9562d38..e33e56e 100644 --- a/src/scribe/mcp/tools/tasks.py +++ b/src/scribe/mcp/tools/tasks.py @@ -45,6 +45,7 @@ from scribe.services import task_logs as task_logs_svc from scribe.services import trash as trash_svc from scribe.services.note_usage import record_pulled from scribe.services.record_refs import refuse_guessed_ids +from scribe.services.text import elide # A work log entry is prose, often long — the discipline asks for what was @@ -64,31 +65,6 @@ _WORK_LOG_ADVICE = ( ) -def elide(text: str, budget: int) -> tuple[str, bool]: - """Cut to `budget` characters from the MIDDLE, keeping both ends. - - A head-only cut — `text[:800]` — decides what a reader sees by character - position, which is uncorrelated with what matters. Prose does not put its - conclusion first: a log entry that opens with what was tried and closes - with "so this shipped in 04775c3" loses exactly the sentence that answers - the question, and the reader cannot tell, because a truncation marker says - that something was removed and never whether it mattered. - - So keep the opening (what this entry is about) AND the closing (where it - landed), and say in between how much went. Two thirds to the head because - that is where the subject is established; the tail needs less to carry a - conclusion. `budget` of 0 means no cut. - """ - if budget <= 0 or len(text) <= budget: - return text, False - head_len = max(1, budget * 2 // 3) - tail_len = max(1, budget - head_len) - omitted = len(text) - head_len - tail_len - head = text[:head_len].rstrip() - tail = text[-tail_len:].lstrip() - return f"{head}\n\n[… {omitted} characters omitted …]\n\n{tail}", True - - def work_log_payload( logs: list, total: int, chars: int, latest_chars: int = _WORK_LOG_LATEST_CHARS ) -> dict: diff --git a/src/scribe/services/embeddings.py b/src/scribe/services/embeddings.py index c29baed..6ac135e 100644 --- a/src/scribe/services/embeddings.py +++ b/src/scribe/services/embeddings.py @@ -686,7 +686,16 @@ async def semantic_search_notes( # to the note's owner, so filtering it would pin every scope to "own" # and leave shared records unreachable by meaning. stmt = ( - select(Note, distance.label("distance")) + # chunk_index/chunk_text ride along so the collapse below can + # say WHICH passage matched. Without them the caller is left + # previewing the head of the body — a span this query has + # already determined is not why the record ranked (#4243). + select( + Note, + distance.label("distance"), + NoteEmbedding.chunk_index, + NoteEmbedding.chunk_text, + ) .select_from(NoteEmbedding) .join(Note, NoteEmbedding.note_id == Note.id) .where( @@ -774,11 +783,23 @@ async def semantic_search_notes( # Recover similarity (1 - distance); order stays highest-first. scored: list[tuple[float, Note]] = [] seen: set[int] = set() - for note, dist in rows: + # The winning row IS the best chunk, by the ordering above — so this is the + # one place that knows which passage earned the hit. Kept beside the score + # rather than returned with it: the return type is list[tuple[float, Note]] + # and ten callers unpack it at ~18 sites, so widening the tuple would be an + # interface change to every one of them with nothing to catch the misses + # (lesson #4207). `report` is the side-channel this function already uses + # for best_available_score. + best_chunk: dict[int, dict] = {} + for note, dist, chunk_index, chunk_text in rows: if int(note.id) in seen: continue seen.add(int(note.id)) scored.append((1.0 - float(dist), note)) + best_chunk[int(note.id)] = { + "index": int(chunk_index), + "text": chunk_text or "", + } # The best score anything reached, bar or no bar. Recorded BEFORE the # filter because a call that returns nothing is exactly when it matters. if report is not None: @@ -797,8 +818,18 @@ async def semantic_search_notes( report["best_available_id"] = int(best[1].id) if best else None scored = [pair for pair in scored if pair[0] >= threshold] if not demote_superseded: - return scored[:limit] - return await _apply_supersession_penalty(scored, limit) + final = scored[:limit] + else: + final = await _apply_supersession_penalty(scored, limit) + # Only for what actually came back, so a caller can key straight off the + # results without carrying chunks for records it never saw. + if report is not None: + report["best_chunk"] = { + int(n.id): best_chunk[int(n.id)] + for _s, n in final + if int(n.id) in best_chunk + } + return final async def backfill_note_embeddings() -> None: diff --git a/src/scribe/services/text.py b/src/scribe/services/text.py new file mode 100644 index 0000000..f966b89 --- /dev/null +++ b/src/scribe/services/text.py @@ -0,0 +1,38 @@ +"""Text shortening for surfaces that cannot show a record whole. + +One function, in one place, because both doors shorten and two copies would +drift — and because the reasoning below is the part that matters and should +not have to be re-derived at each call site. +""" + + +def elide(text: str, budget: int) -> tuple[str, bool]: + """Cut to `budget` characters from the MIDDLE, keeping both ends. + + A head-only cut — `text[:800]` — decides what a reader sees by character + position, which is uncorrelated with what matters. Prose does not put its + conclusion first: a passage that opens with what was attempted and closes + with "so this shipped in 04775c3" loses exactly the sentence that answers + the question. Worse, the reader cannot tell: a truncation marker says that + something was removed, never whether it mattered, so the decision "should + I look deeper?" gets made on evidence selected by length. + + So keep the opening (what this is about) AND the closing (where it landed), + and state in between how much went. Two thirds to the head because that is + where the subject is established; a conclusion needs less room to carry. + + Returns (text, was_cut). A `budget` of 0 or less means no cut. + + This is the fallback, not the goal. Where the system knows WHICH span of a + record is the relevant one — a semantic search knows exactly that, and + stores it (#4243) — show that span and say so. Reach for this only when + nothing identifies a better part than "all of it". + """ + if budget <= 0 or len(text) <= budget: + return text, False + head_len = max(1, budget * 2 // 3) + tail_len = max(1, budget - head_len) + omitted = len(text) - head_len - tail_len + head = text[:head_len].rstrip() + tail = text[-tail_len:].lstrip() + return f"{head}\n\n[… {omitted} characters omitted …]\n\n{tail}", True diff --git a/tests/test_mcp_tool_search.py b/tests/test_mcp_tool_search.py index 951becf..138df7b 100644 --- a/tests/test_mcp_tool_search.py +++ b/tests/test_mcp_tool_search.py @@ -39,23 +39,88 @@ async def test_fable_search_returns_repackaged_results(): r = out["results"][0] assert r["id"] == 1 assert r["title"] == "kafka rebalance" - assert r["body"] == "HPA details" + # The whole body, because it fits — and named as the opening, not as the + # passage that matched, since this call patched the search and so carries + # no chunk. + assert r["excerpt"] == "HPA details" + assert r["excerpt_is"] == "body_opening" + assert r["body_length"] == len("HPA details") assert r["is_task"] is False assert r["tags"] == ["ops"] assert r["similarity"] == pytest.approx(0.93) @pytest.mark.asyncio -async def test_fable_search_body_is_truncated_to_240_chars(): +async def test_search_shows_the_passage_that_matched_not_the_opening(): + """The point of #4243. A record can rank on its sixth paragraph; showing + its first is showing the caller a span the search already judged less + relevant, and letting them decide from it.""" _user_id_ctx.set(7) - long_body = "x" * 500 - fake = fake_note(id=1, title="t", body=long_body) + body = "Chapter one, about nothing. " * 40 + " THE ANSWER IS 04775c3." + fake = fake_note(id=1, title="t", body=body) + + async def _search(*a, **kw): + kw["report"]["best_chunk"] = { + 1: {"index": 3, "text": "THE ANSWER IS 04775c3."} + } + return [(0.5, fake)] + + with patch("scribe.mcp.tools.search.semantic_search_notes", _search): + out = await search(q="answer") + r = out["results"][0] + assert r["excerpt"] == "THE ANSWER IS 04775c3." + assert r["excerpt_is"] == "matched_passage" + assert r["chunk_index"] == 3 + # And the caller is told there is more record behind the passage. + assert r["body_length"] == len(body) + assert "read_full" in r + + +@pytest.mark.asyncio +async def test_a_result_says_whether_its_excerpt_is_the_match_or_the_opening(): + """A caller that cannot tell the two apart cannot judge whether looking + deeper is worth it, which is the only decision this field supports.""" + _user_id_ctx.set(7) + fake = fake_note(id=1, title="t", body="short body") with patch( "scribe.mcp.tools.search.semantic_search_notes", AsyncMock(return_value=[(0.5, fake)]), ): out = await search(q="x") - assert len(out["results"][0]["body"]) == 240 + assert out["results"][0]["excerpt_is"] == "body_opening" + + +@pytest.mark.asyncio +async def test_a_long_excerpt_keeps_both_ends_and_says_how_much_went(): + """The fallback is still an elision, and an elision that drops the tail + drops wherever the conclusion was.""" + _user_id_ctx.set(7) + body = "OPENING. " + ("m" * 3000) + " CLOSING." + fake = fake_note(id=1, title="t", body=body) + with patch( + "scribe.mcp.tools.search.semantic_search_notes", + AsyncMock(return_value=[(0.5, fake)]), + ): + out = await search(q="x") + excerpt = out["results"][0]["excerpt"] + assert excerpt.startswith("OPENING.") + assert excerpt.rstrip().endswith("CLOSING.") + assert "characters omitted" in excerpt + assert out["results"][0]["body_length"] == len(body) + + +@pytest.mark.asyncio +async def test_a_short_record_arrives_whole_and_unmarked(): + """Fragmenting a 200-character note serves nobody.""" + _user_id_ctx.set(7) + fake = fake_note(id=1, title="t", body="all of it") + with patch( + "scribe.mcp.tools.search.semantic_search_notes", + AsyncMock(return_value=[(0.5, fake)]), + ): + out = await search(q="x") + assert out["results"][0]["excerpt"] == "all of it" + assert "read_full" not in out["results"][0] @pytest.mark.asyncio diff --git a/tests/test_pull_telemetry.py b/tests/test_pull_telemetry.py index dfbf399..6ead572 100644 --- a/tests/test_pull_telemetry.py +++ b/tests/test_pull_telemetry.py @@ -224,13 +224,34 @@ def _pulling_getters(): yield module, name, body +def _takes_reading_project(body: str) -> bool: + """Does this function declare `project_id: int = 0`? + + Parsed, not string-matched against the first line. The first version read + `body.split("\n")[0]`, which sees only as far as the first newline — so + adding a parameter to `get_task` wrapped its signature over four lines and + the guard reported a function that DOES take the project as one that does + not. A guard that fails on formatting is a guard that gets appeased by + reflowing the code it was meant to check. + """ + node = ast.parse(body).body[0] + args = node.args + params = list(args.posonlyargs) + list(args.args) + list(args.kwonlyargs) + for arg in params: + if arg.arg != "project_id": + continue + ann = getattr(arg, "annotation", None) + return isinstance(ann, ast.Name) and ann.id == "int" + return False + + def test_every_getter_that_pulls_takes_the_reading_project(): """Asserted on structure (rule 167): a behavioural test cannot see a parameter that was never threaded through.""" missing = [ f"{module}.{name}" for module, name, body in _pulling_getters() - if "project_id: int = 0" not in body.split("\n")[0] + if not _takes_reading_project(body) ] assert not missing, ( f"these getters record a pull but cannot say where the reader was: " diff --git a/tests/test_task_work_log_surface.py b/tests/test_task_work_log_surface.py index 9376173..80d241f 100644 --- a/tests/test_task_work_log_surface.py +++ b/tests/test_task_work_log_surface.py @@ -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 -- 2.54.0 From 6abedb0168ffd7608db0d9759b277b2fe8010aa7 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 08:54:15 -0400 Subject: [PATCH 4/9] test: two tests that had to move with the chunk-carrying select (#4243) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test_chunking::test_search_collapses_chunk_rows_to_best_chunk_per_note feeds rows straight into the real semantic_search_notes, so widening the select to carry chunk_index/chunk_text broke its 2-tuple fakes. I had claimed no unit test did this after grepping test_embeddings.py, which was the wrong file. Rows are 4-tuples now, and the test additionally asserts the winning chunk is REPORTED and not merely used for scoring — the property #4243 exists for, and this is where it belongs, beside the collapse it comes from. test_task_work_log_surface's ACL test asserted "task_logs.user_id" was absent from the compiled statement. It is present in every SELECT as a projected column; the claim was about the WHERE clause. Asserts on stmt.whereclause now, which is what was actually meant and still fails if an owner filter returns. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- tests/test_chunking.py | 20 ++++++++++++++++++-- tests/test_task_work_log_surface.py | 9 ++++++--- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/tests/test_chunking.py b/tests/test_chunking.py index 15861cc..3c56b01 100644 --- a/tests/test_chunking.py +++ b/tests/test_chunking.py @@ -148,23 +148,39 @@ async def test_search_collapses_chunk_rows_to_best_chunk_per_note(): from scribe.services import embeddings as emb note_a, note_b = MagicMock(id=1), MagicMock(id=2) - rows = [(note_a, 0.10), (note_b, 0.20), (note_a, 0.25), (note_a, 0.30)] + # Rows are (Note, distance, chunk_index, chunk_text) — the chunk columns + # ride along so the collapse can report WHICH passage won (#4243). + rows = [ + (note_a, 0.10, 3, "the passage that actually matched"), + (note_b, 0.20, 0, "b's best"), + (note_a, 0.25, 7, "a worse chunk of a"), + (note_a, 0.30, 1, "a worse chunk of a"), + ] result = MagicMock() result.all.return_value = rows session, ctx = _session_ctx() session.execute = AsyncMock(return_value=result) + report: dict = {} with ( patch.object(emb, "async_session", return_value=ctx), patch.object(emb, "get_embedding", AsyncMock(return_value=[0.0] * 384)), ): out = await emb.semantic_search_notes( - 1, "a query", limit=8, demote_superseded=False + 1, "a query", limit=8, demote_superseded=False, report=report ) assert [note.id for _s, note in out] == [1, 2] assert out[0][0] == 1.0 - 0.10 # the BEST chunk's score, not a later one + # And the winning chunk is reported, not merely used for scoring. Without + # this a caller can only preview the head of the body — a span this query + # has already ranked lower than the one that won (#4243). + assert report["best_chunk"][1] == { + "index": 3, "text": "the passage that actually matched", + } + assert report["best_chunk"][2]["index"] == 0 + # --- the write path: one row per chunk (#280 step 3) ------------------------- diff --git a/tests/test_task_work_log_surface.py b/tests/test_task_work_log_surface.py index 80d241f..771f957 100644 --- a/tests/test_task_work_log_surface.py +++ b/tests/test_task_work_log_surface.py @@ -322,9 +322,12 @@ async def test_logs_for_task_is_not_filtered_by_who_wrote_the_entry(): patch("scribe.services.task_logs.async_session", MagicMock(return_value=session)): 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 + # The WHERE clause specifically — `user_id` is a selected COLUMN of the + # row and appears in every SELECT, so asserting against the whole compiled + # statement tests the projection rather than the scoping. + where = str(session.execute.await_args.args[0].whereclause) + assert "task_logs.task_id" in where + assert "task_logs.user_id" not in where @pytest.mark.asyncio -- 2.54.0 From 253fb974f34aa46beb172d495d4360d757f9cd4f Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 09:44:10 -0400 Subject: [PATCH 5/9] feat(retrieval): every semantic search hands on the passage that matched MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #4243 fixed one door. Scribe has three semantic searches over three chunk tables, and all three collapsed chunk rows to the best one per record — each of them KNEW which passage earned the hit, and each dropped it. Every surface downstream then previewed the head of the document instead: a span the search had already scored lower, with nothing saying so. Mechanism, one place: - embeddings.record_best_chunk publishes {id: {index, text}} into `report`. Carried in `report`, NOT the return value: all three return list[tuple[float, Record]] and ~30 sites unpack that pair (lesson #4207). - semantic_search_rules and semantic_search_milestones now select chunk_index/chunk_text and publish the winner, as notes already did. semantic_search_milestones gains `report`, which it had no way to take. - services/text.matched_excerpt is the one choice of span, and excerpt_fields the one result block. Doors keep their own field names — the web renders `snippet`, MCP returns `excerpt` — because renaming a field a frontend reads is a different change from fixing what goes in it. Surfaces: - knowledge.query_knowledge, whose own comment calls it "the human's MAIN search surface", was `(note.body or "")[:200]` on every row alike. Now the matched passage on a search, the opening on a browse, and `snippet_is` saying which. KnowledgeView renders that snippet, so this was live. - search(content_type='milestone') gains `matched` — the plan body stays out, but the passage that matched comes along, because recognising a plan means recognising the part you asked about and a description written at the start need not mention it. - The auto-inject menu and the write-path prior-art menu put the passage under their line. Both were title-only, which answers "does this apply?" for a lesson or snippet (the trigger is IN the title) and not at all for an issue or dev-log. No fallback to the body's opening: on a menu that is preamble dressed as a reason, and once indented it cannot be told apart. Left alone deliberately: the rule arms. A rule hint already renders the rule's TRIGGER, which is written to answer exactly "does this apply to me" and beats a matched chunk at it; and that line's budget was measured at #3851. Adding a passage there would duplicate the trigger and spend the budget twice. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- frontend/src/views/KnowledgeView.vue | 35 +++++- src/scribe/mcp/tools/search.py | 68 +++++----- src/scribe/services/embeddings.py | 80 ++++++++++-- src/scribe/services/knowledge.py | 32 ++++- src/scribe/services/plugin_context.py | 48 +++++++ src/scribe/services/text.py | 63 ++++++++++ tests/test_matched_passage_surfaces.py | 166 +++++++++++++++++++++++++ tests/test_services_plugin_context.py | 86 ++++++++++++- 8 files changed, 527 insertions(+), 51 deletions(-) create mode 100644 tests/test_matched_passage_surfaces.py diff --git a/frontend/src/views/KnowledgeView.vue b/frontend/src/views/KnowledgeView.vue index 81932ec..4c93381 100644 --- a/frontend/src/views/KnowledgeView.vue +++ b/frontend/src/views/KnowledgeView.vue @@ -29,7 +29,17 @@ interface KnowledgeItem { id: number; note_type: "note" | "task" | "process" | "snippet" | "lesson"; title: string; + /** ONE SPAN of the record, never the whole thing. On a search this is the + * passage that actually matched; on a plain browse it is the opening, + * because nothing was matched and no span is better than another. */ snippet: string; + /** Which span `snippet` holds. A reader who cannot tell the matched passage + * from the document's first paragraph cannot tell whether a card that looks + * unrelated really is. */ + snippet_is?: "matched_passage" | "body_opening"; + /** Characters in the whole record, so a long record shown by a short span is + * visible as one. */ + body_length?: number; tags: string[]; project_id: number | null; created_at: string; @@ -181,6 +191,16 @@ const CONTENT_PAGE = 24; // items loaded per sentinel trigger const REFILL_THRESHOLD = 48; // fetch more IDs when queue drops below this const items = ref([]); + +// True only when a search actually returned matched passages — not merely when +// the box has text in it. A keyword-only result set, or an embedder that is +// down, carries `body_opening` rows, and claiming otherwise would be the same +// species of lie this whole change is about (#4243). +const showsMatchedPassages = computed( + () => + searchQuery.value.trim().length > 0 && + items.value.some((i) => i.snippet_is === "matched_passage"), +); const allTags = ref([]); const idQueue = ref([]); // unloaded IDs ready to be content-fetched const idOffset = ref(0); // next offset for ID batch requests @@ -573,8 +593,16 @@ onUnmounted(() => {

Your story is unwritten. Create your first note to begin.

+ +

+ Excerpts below are the passage that matched your search, not the start of each record. +

+ -
+
{ line-height: 1.45; margin: 0; } +.k-excerpt-note { + font-size: 0.8rem; + color: var(--fs-text-tertiary); + margin: 0 0 var(--fs-space-3); +} .k-card-footer { display: flex; align-items: center; diff --git a/src/scribe/mcp/tools/search.py b/src/scribe/mcp/tools/search.py index 027f397..f22b2eb 100644 --- a/src/scribe/mcp/tools/search.py +++ b/src/scribe/mcp/tools/search.py @@ -11,7 +11,7 @@ import time from scribe.mcp._context import current_user_id from scribe.services.access import owner_names_for -from scribe.services.text import elide +from scribe.services.text import MATCHED_PASSAGE, excerpt_fields from scribe.services.embeddings import ( DEFAULT_SIMILARITY_THRESHOLD, semantic_search_milestones, semantic_search_notes, semantic_search_rules, @@ -20,6 +20,14 @@ from scribe.services import rulebooks as rulebooks_svc from scribe.services.retrieval_telemetry import record_retrieval, retrieval_summary +# A matched chunk is at most _CHUNK_CHAR_BUDGET (1400) characters, and it is +# the evidence the ranking was built on — so it is worth more room than the 240 +# characters of document opening this used to send. Elision inside a chunk is +# far less lossy than a head cut of a whole record: the region is already the +# right one. +_EXCERPT_CHARS = 1000 + + 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. @@ -74,10 +82,19 @@ async def _search_milestones(uid: int, q: str, limit: int, project_id: int) -> d Its own result shape, like rules: a milestone is a plan with progress, not a note with a body. The plan itself is left out — get_milestone reads it — - because a search hit is for recognising a plan, and bodies run long. + because a search hit is for recognising a plan, and bodies run long. What + does come along is `matched`: the one passage of the body the query + actually hit, with `matched_is` saying whether it is that passage or + merely the body's opening. Recognising a plan means recognising the part + of it you were asking about, and a description written at the start need + not mention that part (#4243). Not part of content_type="all", whose results are note-shaped. """ - raw = await semantic_search_milestones(uid, q, project_id=project_id or None, limit=limit) + report: dict = {} + raw = await semantic_search_milestones( + uid, q, project_id=project_id or None, limit=limit, report=report, + ) + chunks = report.get("best_chunk") or {} progress: dict[int, dict] = {} if raw: from scribe.services import milestones as milestones_svc @@ -91,6 +108,14 @@ async def _search_milestones(uid: int, q: str, limit: int, project_id: int) -> d "id": m.id, "title": m.title, "description": m.description or "", + # The plan body stays out — get_milestone reads it — but the + # passage that MATCHED comes along, because a plan is + # recognised by the part of it the query was about and a + # description need not mention that part at all (#4243). + **excerpt_fields( + m.body or "", chunks.get(int(m.id)), _EXCERPT_CHARS, + key="matched", + ), "status": m.status, "project_id": m.project_id, "total": progress.get(m.id, {}).get("total", 0), @@ -103,14 +128,6 @@ async def _search_milestones(uid: int, q: str, limit: int, project_id: int) -> d } -# A matched chunk is at most _CHUNK_CHAR_BUDGET (1400) characters, and it is -# the evidence the ranking was built on — so it is worth more room than the 240 -# characters of document opening this used to send. Elision inside a chunk is -# far less lossy than a head cut of a whole record: the region is already the -# right one. -_EXCERPT_CHARS = 1000 - - def result_excerpt(note, chunk: dict | None) -> dict: """The part of a record a caller judges "should I open this?" on. @@ -125,30 +142,13 @@ def result_excerpt(note, chunk: dict | None) -> dict: first, which the search had already judged less relevant, and the caller would decide from that and never know (#4243). - So: show the matched passage when there is one, the body when it fits, and - in either case SAY which of the two this is. A caller that cannot tell an - excerpt from a whole record cannot tell whether looking deeper is worth it, - which is the only decision this field supports. + The choice of span lives in services/text.py, shared with the web's + knowledge search, so the two doors cannot drift on which text a reader is + shown or on whether they are told what it is. """ - body = note.body or "" - matched = (chunk or {}).get("text") or "" - out: dict = {"body_length": len(body)} - - if matched.strip(): - text, cut = elide(matched.strip(), _EXCERPT_CHARS) - out["excerpt"] = text - out["excerpt_is"] = "matched_passage" - if (chunk or {}).get("index") is not None: - out["chunk_index"] = int(chunk["index"]) - else: - # No stored chunk — an un-embedded record, or a caller that passed no - # report. Fall back to the opening, and name it as the opening rather - # than letting it pass for the relevant part. - text, cut = elide(body, _EXCERPT_CHARS) - out["excerpt"] = text - out["excerpt_is"] = "body_opening" - if cut or (out["excerpt_is"] == "matched_passage" and len(matched) < len(body)): - out["read_full"] = "get_note / get_task by id for the whole record." + out = excerpt_fields(note.body or "", chunk, _EXCERPT_CHARS) + if out.get("excerpt_is") == MATCHED_PASSAGE and (chunk or {}).get("index") is not None: + out["chunk_index"] = int(chunk["index"]) return out diff --git a/src/scribe/services/embeddings.py b/src/scribe/services/embeddings.py index 6ac135e..d706a38 100644 --- a/src/scribe/services/embeddings.py +++ b/src/scribe/services/embeddings.py @@ -577,6 +577,30 @@ GLOBAL_NOTE_TYPES: tuple[str, ...] = ("lesson",) # write telemetry pass a dict and read `best_available_score` back out of it. +def record_best_chunk(report: dict | None, chunks: dict[int, dict]) -> None: + """Publish the winning chunk per record into `report["best_chunk"]`. + + Every semantic search here collapses several chunk rows to the best one per + record, which means each of them KNOWS which passage earned the hit — and + each of them used to drop it, leaving every caller to preview the head of + the document instead. The head is a different span, one the search has + already scored lower, and nothing in the result said so (#4243). + + It rides in `report` rather than in the return value because all three + searches return `list[tuple[float, Record]]` and roughly thirty sites + unpack that pair; widening it would be an interface change to every one of + them with nothing to catch a miss (lesson #4207). `report` is already the + side-channel these functions use for `searched` and `best_available_score`, + so this adds a key to a channel callers already open. + + Shape: {record_id: {"index": int, "text": str}}. A caller that passed no + report simply doesn't get it, and every consumer falls back to the body. + """ + if report is None: + return + report["best_chunk"] = chunks + + async def semantic_search_notes( user_id: int, query: str, @@ -823,12 +847,11 @@ async def semantic_search_notes( final = await _apply_supersession_penalty(scored, limit) # Only for what actually came back, so a caller can key straight off the # results without carrying chunks for records it never saw. - if report is not None: - report["best_chunk"] = { - int(n.id): best_chunk[int(n.id)] - for _s, n in final - if int(n.id) in best_chunk - } + record_best_chunk(report, { + int(n.id): best_chunk[int(n.id)] + for _s, n in final + if int(n.id) in best_chunk + }) return final @@ -1075,7 +1098,12 @@ async def semantic_search_rules( async with async_session() as session: rows = (await session.execute( - select(Rule, distance.label("distance")) + select( + Rule, + distance.label("distance"), + RuleEmbedding.chunk_index, + RuleEmbedding.chunk_text, + ) .select_from(RuleEmbedding) .join(Rule, RuleEmbedding.rule_id == Rule.id) .outerjoin(RulebookTopic, Rule.topic_id == RulebookTopic.id) @@ -1098,11 +1126,22 @@ async def semantic_search_rules( return [] best: dict[int, tuple[float, object]] = {} - for rule, dist in rows: + # Which chunk won, kept beside the score it won with — a rule's `why` and + # `how_to_apply` are long, and a caller shown only the head cannot see the + # clause that actually matched (#4243). + won: dict[int, dict] = {} + for rule, dist, chunk_index, chunk_text in rows: score = 1.0 - float(dist) if rule.id not in best or score > best[rule.id][0]: best[rule.id] = (score, rule) + won[int(rule.id)] = { + "index": int(chunk_index), "text": chunk_text or "", + } ranked = sorted(best.values(), key=lambda pair: pair[0], reverse=True) + kept = [pair for pair in ranked if pair[0] >= threshold][:limit] + record_best_chunk(report, { + int(r.id): won[int(r.id)] for _s, r in kept if int(r.id) in won + }) if report is not None: # See the sibling search: absent means the search never ran (#3765). report["searched"] = True @@ -1110,7 +1149,7 @@ async def semantic_search_rules( best = ranked[0] if ranked else None report["best_available_score"] = best[0] if best else None report["best_available_id"] = int(best[1].id) if best else None - return [pair for pair in ranked if pair[0] >= threshold][:limit] + return kept async def backfill_rule_embeddings() -> None: @@ -1218,6 +1257,7 @@ async def semantic_search_milestones( status: str | None = None, limit: int = 5, threshold: float = _SIMILARITY_THRESHOLD, + report: dict | None = None, ) -> list[tuple[float, "Milestone"]]: """Return up to *limit* (score, milestone) pairs most like *query*. @@ -1254,7 +1294,12 @@ async def semantic_search_milestones( scope = Milestone.user_id == user_id async with async_session() as session: rows = (await session.execute( - select(Milestone, distance.label("distance")) + select( + Milestone, + distance.label("distance"), + MilestoneEmbedding.chunk_index, + MilestoneEmbedding.chunk_text, + ) .select_from(MilestoneEmbedding) .join(Milestone, MilestoneEmbedding.milestone_id == Milestone.id) .where( @@ -1270,12 +1315,23 @@ async def semantic_search_milestones( return [] best: dict[int, tuple[float, object]] = {} - for milestone, dist in rows: + # A milestone's `body` IS the plan, and search results show its short + # `description` — so a match on the design was previewed by a sentence that + # need not mention it. The winning chunk is what the caller should see. + won: dict[int, dict] = {} + for milestone, dist, chunk_index, chunk_text in rows: score = 1.0 - float(dist) if milestone.id not in best or score > best[milestone.id][0]: best[milestone.id] = (score, milestone) + won[int(milestone.id)] = { + "index": int(chunk_index), "text": chunk_text or "", + } ranked = sorted(best.values(), key=lambda pair: pair[0], reverse=True) - return [pair for pair in ranked if pair[0] >= threshold][:limit] + kept = [pair for pair in ranked if pair[0] >= threshold][:limit] + record_best_chunk(report, { + int(m.id): won[int(m.id)] for _s, m in kept if int(m.id) in won + }) + return kept async def backfill_milestone_embeddings() -> None: diff --git a/src/scribe/services/knowledge.py b/src/scribe/services/knowledge.py index dd7959d..8e053c9 100644 --- a/src/scribe/services/knowledge.py +++ b/src/scribe/services/knowledge.py @@ -25,6 +25,7 @@ from scribe.models import async_session from scribe.models.note import Note from scribe.models.base import iso from scribe.services.access import browsable_notes_clause, readable_notes_clause +from scribe.services.text import excerpt_fields logger = logging.getLogger(__name__) @@ -211,12 +212,30 @@ def _verification_clause(value: str): ) -def _note_to_item(note: Note) -> dict: +def _note_to_item(note: Note, chunks: dict[int, dict] | None = None) -> dict: + """One row for a browse or search listing. + + `chunks` is `report["best_chunk"]` from a semantic search, when this row + came from one. With it the card shows the passage that MATCHED; without it + — a plain listing, where nothing was matched and so no span is better than + any other — it shows the record's opening. `snippet_is` says which, on + every row, so the two never have to be told apart by guessing. + + This was `(note.body or "")[:_SNIPPET_LEN]` on every row alike: the head of + the document, on the human's main search surface, with nothing marking the + cut. A record matched on its sixth paragraph was shown its first, which the + search had already scored lower (#4243). + """ item: dict = { "id": note.id, "note_type": note.note_type or "note", "title": note.title, - "snippet": (note.body or "")[:_SNIPPET_LEN], + **excerpt_fields( + note.body or "", + (chunks or {}).get(int(note.id)), + _SNIPPET_LEN, + key="snippet", + ), "tags": note.tags or [], "project_id": note.project_id, # These lists now include records shared with the caller, so the client @@ -507,6 +526,9 @@ async def _semantic_knowledge_search( # record would be findable by wording and invisible by meaning — which is the # case a semantic search exists to serve. semantic_notes: list[Note] = [] + # Filled by the search below; stays empty when the embedder is down or the + # call raises, in which case every row falls back to its opening. + _semantic_report: dict = {} try: from scribe.services.embeddings import ( INTERACTIVE_SEARCH_THRESHOLD, @@ -519,6 +541,7 @@ async def _semantic_knowledge_search( user_id=user_id, scope="read", query=q, + report=_semantic_report, limit=min(200, limit * 4), # The shared interactive floor — this was a bare `0.3` while # routes/search.py had the same number as a commented constant, the @@ -571,7 +594,10 @@ async def _semantic_knowledge_search( total = len(merged) page_items = merged[offset: offset + limit] - return [_note_to_item(n) for n in page_items], total + return [ + _note_to_item(n, _semantic_report.get("best_chunk")) + for n in page_items + ], total async def get_knowledge_tags(user_id: int, note_type: str | None = None) -> list[str]: diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 5f7ed2b..43bc94c 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -40,12 +40,31 @@ from scribe.services.retrieval_surfaces import ( ) from scribe.services.retrieval_telemetry import record_retrieval from scribe.services.settings import get_setting +from scribe.services.text import elide logger = logging.getLogger(__name__) # Defensive cap below Claude Code's 10k additionalContext limit. _MAX_CHARS = 9000 +# Max chars of the matched passage shown under an injected menu line. +# +# The menu used to be titles alone, on the reasoning that its job is AWARENESS — +# make the agent know the record exists and reach for it, not dump it. That +# holds for a lesson or a snippet, whose title carries its trigger by +# construction ("what — when it applies"). It does not hold for an issue, a +# dev-log or a plain note, where the title is a headline and the reason this +# record matched is a sentence somewhere inside it. The reader was being asked +# "is this worth opening?" and handed the one part of the record guaranteed not +# to answer it. +# +# 200 rather than more because this is a menu: eight lines at 200 is ~1.6KB, +# which buys the decision without turning an awareness push into a dump. It is +# the PASSAGE THAT MATCHED, not the record's opening — the search already knows +# which one that is and used to throw it away (#4243) — so 200 characters here +# are worth far more than 200 characters of preamble. +_MENU_PASSAGE_CHARS = 200 + # Max chars of a Process body to fold into the auto-surface description. _PROC_PREVIEW_CHARS = 200 @@ -1095,6 +1114,10 @@ async def build_autoinject_hint( # which is worse than never having surfaced it. One query for the whole menu. stale = await superseded_ids([int(n.id) for _s, n in kept]) + # From THIS arm's own search (`_rep_ai`), so a chunk is only ever paired + # with the query that actually matched it. + menu_chunks = _rep_ai.get("best_chunk") or {} + note_ids: list[int] = [] for score, note in kept: note_ids.append(int(note.id)) @@ -1113,6 +1136,16 @@ async def build_autoinject_hint( who = owners.get(int(note.user_id)) or "another user" line += f" — shared by {who}, treat as a suggestion" lines.append(line) + # The passage that earned the line, indented under it. Absent when the + # record has no stored chunk — an un-embedded row, or the reserved + # lesson and reuse slots, which are fetched by their own queries and so + # are not in this search's report. No fallback to the body's opening: + # on a menu that would be a line of preamble dressed as a reason, and a + # reader cannot tell the two apart once they are indented identically. + passage = (menu_chunks.get(int(note.id)) or {}).get("text") or "" + if passage.strip(): + short, _cut = elide(" ".join(passage.split()), _MENU_PASSAGE_CHARS) + lines.append(f"> ↳ {short}") # Records what SURVIVED the margin gate, not what the ranker returned — the # menu the agent actually saw. retrieval_logs already holds the full @@ -2035,6 +2068,11 @@ async def build_write_path_hint( # floor. Applying the floor after this would throw away the best queries. if query: query = concept_query(query) or query + # Declared out here because the search below is conditional — this arm runs + # only when the menu has room AND a query survived the floor. The render + # loop is not conditional, so it needs something to read either way, and an + # empty mapping means every line falls back to its title alone. + wp_chunks: dict[int, dict] = {} if remaining > 0 and query: t0 = time.perf_counter() # Pulled-and-already-listed ids stay in the query (as evidence for @@ -2089,6 +2127,7 @@ async def build_write_path_hint( scope="browse", report=_rep_wp, ) + wp_chunks = _rep_wp.get("best_chunk") or {} resembles = { int(note.id): float(score) for score, note in hits if int(note.id) in pulled @@ -2337,6 +2376,15 @@ async def build_write_path_hint( for item, marker, owner, foreign_lang in rendered: note_ids.append(int(item["id"])) lines.append(_prior_art_line(item, marker, owner, foreign_lang)) + # Only the semantically-matched lines carry a passage. The records-this- + # file lines came from a LOCATION lookup — nothing was matched, so there + # is no matching passage and the body's opening would be a fabricated + # reason. Absence here is meaningful: a line with no passage under it is + # one that earned its place by where it lives, not by what it says. + passage = (wp_chunks.get(int(item["id"])) or {}).get("text") or "" + if passage.strip(): + short, _cut = elide(" ".join(passage.split()), _MENU_PASSAGE_CHARS) + lines.append(f"> ↳ {short}") if stamped: lines.append(_stamp_line(path, stamped)) diff --git a/src/scribe/services/text.py b/src/scribe/services/text.py index f966b89..0d2ce3c 100644 --- a/src/scribe/services/text.py +++ b/src/scribe/services/text.py @@ -36,3 +36,66 @@ def elide(text: str, budget: int) -> tuple[str, bool]: head = text[:head_len].rstrip() tail = text[-tail_len:].lstrip() return f"{head}\n\n[… {omitted} characters omitted …]\n\n{tail}", True + + +# What a search result shows a reader, as one decision made in one place. +# +# Every door onto a semantic search faces the same question — which span of a +# record do I show someone deciding whether to open it? — and each used to +# answer it separately with a bare head cut of a different length: 240 +# characters in the MCP search, 200 in the web's knowledge search. Both showed +# the document's OPENING, which is not the span that matched and not the span +# the ranking was built on. +# +# Doors keep their own field names (the web UI renders `snippet`, the MCP +# surface returns `excerpt`), because renaming a field a frontend consumes is +# a separate change from fixing which text goes in it. What they share is this +# function: the choice of span, and the obligation to say which span it is. + +MATCHED_PASSAGE = "matched_passage" +BODY_OPENING = "body_opening" + + +def matched_excerpt( + body: str, chunk: dict | None, budget: int +) -> tuple[str, str, bool]: + """Pick the span to show, and say which span it is. + + `chunk` is a `report["best_chunk"]` entry from one of the semantic + searches — `{"index": int, "text": str}` — or None when the caller ran no + semantic search, passed no report, or the record has no embedding row. + + Returns `(text, kind, was_cut)` where `kind` is MATCHED_PASSAGE or + BODY_OPENING. The kind is not decoration: a reader who cannot tell the + passage that earned the hit from the first paragraph of the document + cannot tell whether a thin-looking result is genuinely thin, and the whole + point of the excerpt is to support exactly that judgement. + + A short record comes back whole either way — fragmenting a 200-character + note serves nobody, and its opening IS its content. + """ + passage = (chunk or {}).get("text") or "" + if passage.strip(): + text, cut = elide(passage.strip(), budget) + return text, MATCHED_PASSAGE, cut + text, cut = elide(body or "", budget) + return text, BODY_OPENING, cut + + +def excerpt_fields( + body: str, chunk: dict | None, budget: int, *, key: str = "excerpt" +) -> dict: + """`matched_excerpt` as the block a result row carries. + + `key` names the text field so a door can keep the name its consumers + already read. The companion keys are derived from it, so a row never ends + up with an excerpt under one name and its label under another. + """ + text, kind, cut = matched_excerpt(body, chunk, budget) + out = {key: text, f"{key}_is": kind, "body_length": len(body or "")} + if cut or (kind == MATCHED_PASSAGE and len(text) < len(body or "")): + out["read_full"] = ( + "This is one span of a longer record. Open it by id for the whole " + "thing rather than judging from what is here." + ) + return out diff --git a/tests/test_matched_passage_surfaces.py b/tests/test_matched_passage_surfaces.py new file mode 100644 index 0000000..cf628f8 --- /dev/null +++ b/tests/test_matched_passage_surfaces.py @@ -0,0 +1,166 @@ +"""Every semantic search hands on the passage that matched (#4243, #4250). + +Three searches collapse chunk rows to the best one per record, so each of them +KNOWS which passage earned the hit. Each used to drop it, leaving every door to +preview the head of the document instead — a span the search had already scored +lower, with nothing saying so. + +These pin the mechanism (`report["best_chunk"]` from all three searches) and +each surface that reads it, because the failure mode is silent: a door that +quietly reverts to the body's opening still returns a plausible-looking string +and no test that only checks "a preview exists" would notice. +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.services import embeddings as emb +from scribe.services.text import ( + BODY_OPENING, + MATCHED_PASSAGE, + excerpt_fields, + matched_excerpt, +) +from tests.helpers import make_mock_session + + +def _searching(rows): + """A patched session whose one query returns `rows` — the shared + make_mock_session (#2834) rather than a third local copy of the + __aenter__/__aexit__ dance.""" + session = make_mock_session() + result = MagicMock() + result.all.return_value = rows + session.execute = AsyncMock(return_value=result) + return session + + +# --------------------------------------------------------------------------- +# The shared choice of span +# --------------------------------------------------------------------------- + +def test_the_matched_passage_wins_over_the_opening(): + body = "An opening about nothing much. " * 20 + " THE ANSWER." + text, kind, _cut = matched_excerpt(body, {"index": 4, "text": "THE ANSWER."}, 1000) + assert text == "THE ANSWER." + assert kind == MATCHED_PASSAGE + + +def test_without_a_chunk_the_opening_is_named_as_the_opening(): + """The fallback is legitimate — a plain listing matched nothing — but it + must not pass for the passage that matched.""" + text, kind, _cut = matched_excerpt("just a body", None, 1000) + assert kind == BODY_OPENING + + +def test_an_empty_chunk_is_not_mistaken_for_a_passage(): + """A record embedded from its title alone stores an empty body chunk; + rendering that as "the passage that matched" would be a blank line + presented as evidence.""" + _t, kind, _c = matched_excerpt("real body", {"index": 0, "text": " "}, 1000) + assert kind == BODY_OPENING + + +def test_the_field_names_travel_together(): + """A door renames the text field to keep its consumers working; the label + has to follow it, or a row carries an excerpt under one name and its + meaning under another.""" + out = excerpt_fields("b" * 500, {"index": 1, "text": "hit"}, 100, key="snippet") + assert out["snippet"] == "hit" + assert out["snippet_is"] == MATCHED_PASSAGE + assert out["body_length"] == 500 + assert "read_full" in out + + +def test_a_record_shown_whole_advertises_nothing_further(): + out = excerpt_fields("short", None, 1000) + assert out["excerpt"] == "short" + assert "read_full" not in out + + +# --------------------------------------------------------------------------- +# All three searches publish the winning chunk +# --------------------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_rule_search_reports_the_clause_that_matched(): + """A rule's `why` and `how_to_apply` run long. A caller shown only the head + cannot see the clause the query actually hit.""" + r1, r2 = MagicMock(id=1), MagicMock(id=2) + session = _searching([ + (r1, 0.10, 2, "the clause that matched"), + (r2, 0.20, 0, "r2 best"), + (r1, 0.40, 9, "a worse clause of r1"), + ]) + report: dict = {} + with ( + patch.object(emb, "async_session", return_value=session), + patch.object(emb, "get_embedding", AsyncMock(return_value=[0.0] * 384)), + patch.object(emb, "can_read_project", AsyncMock(return_value=True)), + ): + out = await emb.semantic_search_rules(1, "q", limit=5, threshold=0.0, + report=report) + assert [r.id for _s, r in out] == [1, 2] + assert report["best_chunk"][1] == {"index": 2, "text": "the clause that matched"} + + +@pytest.mark.asyncio +async def test_milestone_search_reports_the_passage_of_the_plan_that_matched(): + """A milestone's body IS the plan and search shows its short description, + which need not mention the part the query was about.""" + m1 = MagicMock(id=7) + session = _searching([(m1, 0.15, 5, "step 6 — the acceptance case")]) + report: dict = {} + with ( + patch.object(emb, "async_session", return_value=session), + patch.object(emb, "get_embedding", AsyncMock(return_value=[0.0] * 384)), + patch.object(emb, "can_read_project", AsyncMock(return_value=True)), + ): + out = await emb.semantic_search_milestones( + 1, "acceptance", limit=5, threshold=0.0, report=report, + ) + assert [m.id for _s, m in out] == [7] + assert report["best_chunk"][7]["text"] == "step 6 — the acceptance case" + + +@pytest.mark.asyncio +async def test_a_caller_that_passes_no_report_still_works(): + """Every one of these searches fails open by design — a recall aid must + never break the call it serves — and that includes the chunk channel.""" + session = _searching([(MagicMock(id=1), 0.1, 0, "text")]) + with ( + patch.object(emb, "async_session", return_value=session), + patch.object(emb, "get_embedding", AsyncMock(return_value=[0.0] * 384)), + patch.object(emb, "can_read_project", AsyncMock(return_value=True)), + ): + out = await emb.semantic_search_milestones(1, "q", limit=5, threshold=0.0) + assert len(out) == 1 + + +def test_record_best_chunk_on_no_report_is_a_no_op(): + emb.record_best_chunk(None, {1: {"index": 0, "text": "x"}}) # must not raise + + +# --------------------------------------------------------------------------- +# Only what survived the bar is published +# --------------------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_chunks_are_published_only_for_results_that_came_back(): + """Keyed off the returned rows, so a caller can look up every result it has + and never holds passages for records it was not shown.""" + keep, drop = MagicMock(id=1), MagicMock(id=2) + session = _searching([ + (keep, 0.10, 0, "kept"), + (drop, 0.95, 0, "below the bar"), + ]) + report: dict = {} + with ( + patch.object(emb, "async_session", return_value=session), + patch.object(emb, "get_embedding", AsyncMock(return_value=[0.0] * 384)), + patch.object(emb, "can_read_project", AsyncMock(return_value=True)), + ): + await emb.semantic_search_milestones( + 1, "q", limit=5, threshold=0.5, report=report, + ) + assert set(report["best_chunk"]) == {1} diff --git a/tests/test_services_plugin_context.py b/tests/test_services_plugin_context.py index 0d893be..3fc6a63 100644 --- a/tests/test_services_plugin_context.py +++ b/tests/test_services_plugin_context.py @@ -78,8 +78,12 @@ async def test_build_autoinject_hint_titles_only_with_margin_gate(): assert out["note_ids"] == [11, 22] assert '#11 [note] "Pool sizing decision" (0.80)' in out["context"] assert "#33" not in out["context"] - # Title-first: no body text, ever. + # Title-first when the search reports no matched passage — which is this + # test, whose mock returns bare (score, note) pairs and fills no report. + # A record whose passage IS known gets it on a second line; that is + # test_the_menu_shows_the_passage_that_matched below. assert "get_note(id)" in out["context"] + assert "↳" not in out["context"] # Telemetry fired for BOTH retrievals this path runs: the scored menu and # the reuse-slot query competing against it. The slot's query used to be # the one unlogged retrieval on this path — the hit it displaced was in @@ -552,3 +556,83 @@ async def test_the_config_stand_in_carries_every_key_the_real_one_does(): "a key the real config has and the stand-in does not turns an arm " "into a silent no-op under test" ) + + +# ─── the passage that matched travels onto the menu (#4243, #4250) ─────────── + + +@pytest.mark.asyncio +async def test_the_menu_shows_the_passage_that_matched(): + """A title is a headline. For a lesson or a snippet it carries the trigger + and answers "does this apply to me"; for an issue or a dev-log the reason + this record matched is a sentence somewhere inside it, and the reader was + being handed the one part guaranteed not to say so.""" + from scribe.services import plugin_context as pc + + hits = [(0.80, fake_note(id=11, title="Pool sizing decision", user_id=1))] + + # Only the FIRST call is the menu's own search; the reuse and lesson slots + # run their own queries afterwards and must not contribute chunks, which is + # also what makes the count assertion below deterministic. `report` is + # optional on this interface, so it is written only when one was passed. + calls: list[int] = [] + + async def _menu_search(*_a, **kw): + calls.append(1) + if len(calls) > 1: + return [] + if kw.get("report") is not None: + kw["report"]["best_chunk"] = { + 11: {"index": 3, "text": "we set max_overflow to 5 after the leak"} + } + return hits + + with patch.object(pc, "get_autoinject_config", + AsyncMock(return_value={"enabled": True, "threshold": 0.55, + "top_k": 3})), \ + patch.object(pc, "semantic_search_notes", _menu_search), \ + patch.object(pc, "record_retrieval", MagicMock()): + out = await pc.build_autoinject_hint(1, "pool", project_id=2) + + assert "we set max_overflow to 5 after the leak" in out["context"] + # Indented under its line, so the menu still reads as a list of records + # rather than a wall of prose. + assert "> ↳ we set max_overflow" in out["context"] + + +@pytest.mark.asyncio +async def test_a_record_with_no_stored_chunk_gets_no_invented_passage(): + """The reserved lesson and reuse slots are fetched by their own queries, so + they are absent from this search's report. Falling back to the body's + opening would put a line of preamble under them dressed as the reason they + matched — and once indented identically, a reader cannot tell the two + apart.""" + from scribe.services import plugin_context as pc + + hits = [(0.80, fake_note(id=11, title="Has a chunk", user_id=1)), + (0.78, fake_note(id=22, title="Has none", user_id=1, + body="A long body whose opening says nothing."))] + + calls: list[int] = [] + + async def _menu_search(*_a, **kw): + calls.append(1) + if len(calls) > 1: + return [] + if kw.get("report") is not None: + kw["report"]["best_chunk"] = { + 11: {"index": 0, "text": "the real reason"} + } + return hits + + with patch.object(pc, "get_autoinject_config", + AsyncMock(return_value={"enabled": True, "threshold": 0.55, + "top_k": 3})), \ + patch.object(pc, "semantic_search_notes", _menu_search), \ + patch.object(pc, "record_retrieval", MagicMock()): + out = await pc.build_autoinject_hint(1, "q", project_id=2) + + assert "the real reason" in out["context"] + assert "A long body whose opening" not in out["context"] + # Exactly one passage line, for the one record that had a passage. + assert out["context"].count("↳") == 1 -- 2.54.0 From 0ec499d9b473eabd6e4e4bccb057e353a9487f4b Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 09:46:33 -0400 Subject: [PATCH 6/9] test: the milestone result shape moved, and its fake had a mock for a body MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test_milestone_search_is_its_own_shape asserts the result dict exactly, so adding `matched` / `matched_is` / `body_length` broke it — which is the test doing its job. Its fake was a bare MagicMock, so `m.body` autovivified: `matched` came out as a mock object and `body_length` as 0, and nothing in the old assertion touched either. Lesson #2833 is about exactly this, and the failure output is what surfaced it. The fake now carries a real body string and a report with the chunk that matched, so the assertion is about the product rather than about MagicMock's attribute behaviour. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- tests/test_mcp_tool_search.py | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/tests/test_mcp_tool_search.py b/tests/test_mcp_tool_search.py index 138df7b..88d4ee1 100644 --- a/tests/test_mcp_tool_search.py +++ b/tests/test_mcp_tool_search.py @@ -211,16 +211,36 @@ async def test_milestone_search_is_its_own_shape_and_scopes_to_the_project(): from unittest.mock import MagicMock _user_id_ctx.set(7) + # `body` is a REAL string, not left to MagicMock's attribute autovivication: + # the result now reads it, and a mock body would make `matched` a mock + # object and `body_length` zero while the assertion still looked green + # (lesson #2833). ms = MagicMock(id=339, title="M3 — Metadata", description="works, editions", - status="active", project_id=30) - found = AsyncMock(return_value=[(0.81, ms)]) + status="active", project_id=30, + body="Step 4 — the metadata editions table.") + summary = AsyncMock(return_value=[{"id": 339, "total": 0, "completed": 0}]) - with patch("scribe.mcp.tools.search.semantic_search_milestones", found), \ + seen: dict = {} + + async def _milestone_search(*_a, **kw): + seen.update(kw) + kw["report"]["best_chunk"] = { + 339: {"index": 1, "text": "Step 4 — the metadata editions table."} + } + return [(0.81, ms)] + + with patch("scribe.mcp.tools.search.semantic_search_milestones", + _milestone_search), \ patch("scribe.services.milestones.get_project_milestone_summary", summary): out = await search(q="book metadata", content_type="milestone", project_id=30) - assert found.await_args.kwargs["project_id"] == 30 + assert seen["project_id"] == 30 assert out["results"] == [{ "id": 339, "title": "M3 — Metadata", "description": "works, editions", + # The plan body stays out; the passage that matched comes along, and + # says which of the two it is (#4243). + "matched": "Step 4 — the metadata editions table.", + "matched_is": "matched_passage", + "body_length": len("Step 4 — the metadata editions table."), "status": "active", "project_id": 30, "total": 0, "completed": 0, "similarity": 0.81, }] -- 2.54.0 From 5fb41af9b01e95ee26c197724e2ceef72c1e73d4 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 11:12:48 -0400 Subject: [PATCH 7/9] feat(search): the agent's search can ask for every kind the corpus has (#4250) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The engine took `note_type` and `task_kind` all along. What was missing was a way to say them: the MCP tool's `content_type` knew `note`, `task` and `all`, and `/api/search` knew the same two — so an agent could not ask "has this snippet already been recorded" or "what lessons apply here" without searching everything and reading past the rest. Browse offered nine kinds from the same data. The cause is that each door kept its own map. `_FACETS` in services/knowledge is where a kind is declared, and #3161 made adding one a single edit by generating the SQL filter, the Python predicate and the door's validation from it — but the two search doors were written before that and never joined. So this adds the third dialect, `search_filters_for`, and one composition over it, `content_type_filters`, and both doors now derive instead of listing. Two names keep a meaning of their own, and the docstrings say so: `all` is no filter, and `note` is BROAD — any non-task, snippets and lessons included — where the browse facet of the same name is narrow (`note_type == 'note'`). They are left different deliberately; narrowing this one would stop returning snippets to every caller that already asks this way. An unrecognised kind is now refused rather than answered. Both doors used to fall through: the MCP tool into a filter matching no row, the route into no filter at all, so `?content_type=snippets` returned the whole corpus while looking like a narrowed search. An empty result set is a claim — "the corpus holds nothing like this" — and an agent acts on that claim by building the thing it could not find, so a typo must not be able to make it. The docstring is the agent-facing contract (#2846), and a test now holds it to the table: every kind `_FACETS` declares has to appear in it, because a filter an agent has not been told about is unreachable however well it is wired. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/mcp/tools/search.py | 42 +++++++++++++-- src/scribe/routes/search.py | 25 ++++----- src/scribe/services/knowledge.py | 63 +++++++++++++++++++++++ tests/test_mcp_tool_search.py | 87 +++++++++++++++++++++++++++++++- tests/test_search_route.py | 52 +++++++++++++++---- 5 files changed, 242 insertions(+), 27 deletions(-) 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) -- 2.54.0 From aa95c109eac46f6c045701b420751e5a324bf654 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 11:20:27 -0400 Subject: [PATCH 8/9] feat(retrieval): a task's work logs join the document it is embedded as (#4251) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 1 of #4251. A work log is the richest prose Scribe holds about WHY something is the way it is — written during the work, recording what was tried and ruled out. #4241 made it readable from the agent's door. It was still not findable, so "has anyone tried this approach?" — precisely the question a log answers — could not reach one. The cost is not hypothetical: #4208 was rebuilt in this session because its logs were unreachable. THE DESIGN QUESTION the issue left open was whether logs embed as part of their task's document or as rows of their own. As their own rows, a hit has to be resolved back to a task to be worth anything, and it needs a fourth search, a fourth result shape and a fourth arm. As part of the task, the objection is that a long log drowns a short title. That objection was true before #280 and is not true now. Chunking made one record into one vector per section, so each log becomes its own title-anchored chunk, scored separately, and the task's own prose keeps the chunk it always had — a task is as findable as its best-matching log rather than as the average of everything in it. The other half is this session's other build: a search hands back the chunk that won (#4243), so a hit earned by a log shows that log's passage under the task's title. Without that a reader would have got body[:240] of the task — the opening of a record whose relevance lives three hundred lines further down. So `task_document(title, body, logs)` sits beside `rule_document`: a synthesised embed-time shape, because the stored record is the task row and the logs live in their own table, so the document that should be searchable exists nowhere until it is built. A task with no logs is returned untouched — most notes are not tasks and most tasks carry no log, and their vectors are the corpus every tuned number here was measured against. CHUNKER_VERSION 1 → 2, and its comment now says what the version actually means. It used to read "whenever chunk_document's output can change for the same input", which this change would slip past: `chunk_document` is untouched and every task with a log now embeds differently while its title, body and the chunker all stand still. The invariant is the document a record is embedded as. The startup backfill re-embeds on that. Create, edit and delete of a log all refresh the task through `embed_note`, the one path every writer shares — an edited log whose vectors still carry its old wording keeps matching what it no longer says. No new kind enters the auto-inject menu: tasks were always in it, and this makes recall on them better rather than changing what the menu spans. The calibration stamp will now report shape_version 2 against numbers measured at 1, which is exactly the report #4104 built it to make. Also promoted `session_returning` into tests/helpers beside `make_mock_session` (#2834) — two files had spelled it out identically and a third was about to. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/services/embeddings.py | 127 ++++++++++++- src/scribe/services/task_logs.py | 34 ++++ tests/helpers.py | 18 ++ tests/test_chunking.py | 36 +++- tests/test_embedding_yields_to_a_delete.py | 26 ++- tests/test_services_dedup.py | 29 ++- tests/test_task_document_shape.py | 200 +++++++++++++++++++++ 7 files changed, 439 insertions(+), 31 deletions(-) create mode 100644 tests/test_task_document_shape.py diff --git a/src/scribe/services/embeddings.py b/src/scribe/services/embeddings.py index d706a38..7927fd6 100644 --- a/src/scribe/services/embeddings.py +++ b/src/scribe/services/embeddings.py @@ -23,6 +23,7 @@ from sqlalchemy import delete, or_, select from scribe.models import async_session from scribe.models.embedding import NoteEmbedding, RuleEmbedding from scribe.models.note import Note +from scribe.models.task_log import TaskLog from scribe.services.access import can_read_project, notes_visibility_clause if TYPE_CHECKING: # resolves forward refs without importing at runtime @@ -271,11 +272,20 @@ def untrigger_title(title: str | None, trigger: str | None) -> str: # search. The fix is the document shape: one vector per meaningful chunk, and a # record is as findable as its best-matching section. -# Bumped whenever chunk_document's output can change for the same input. Stored -# on every note_embeddings row so the startup backfill can re-embed exactly the -# notes whose stored shape is stale — a version comparison instead of the table -# wipe migrations 0067/0077 had to do. -CHUNKER_VERSION = 1 +# Bumped whenever THE DOCUMENT A RECORD IS EMBEDDED AS can change for a record +# that itself has not changed. Stored on every note_embeddings row so the +# startup backfill re-embeds exactly the stale notes — a version comparison +# instead of the table wipe migrations 0067/0077 had to do. +# +# Stated that way rather than as "chunk_document's output for the same input", +# which is what it used to say: `chunk_document` is only the last step, and +# version 2 moves without touching it. A task's document now carries its work +# logs (#4251), so every task that has one embeds differently than it did while +# its title, body and the chunker are all untouched — exactly the case the +# narrower wording would have read as "nothing to re-embed". +# +# 1 → 2: work logs joined the task document. +CHUNKER_VERSION = 2 # The public name of the space every score lives in, and the two facts that @@ -483,6 +493,47 @@ async def _claim_parent_row(session, id_column, row_id: int, label: str) -> bool return True +async def _work_log_sections(note_id: int) -> list[tuple[object, str | None]]: + """A task's work logs, oldest first, for its embedded document (#4251). + + Reads the table directly rather than through `task_logs.logs_for_task`, + because that function asks a PERMISSION question — may this user read this + task — and there is no user here. An index build acts for the record, and + the record's vectors carry the owner's `user_id`, so the access decision is + made once at search time by the clause that already scopes every hit. + + That also settles what happens on a shared task: a collaborator's log is + part of the task's document, so it becomes findable by everyone who can + read the task and by nobody else. The same answer `logs_for_task` gives a + reader (#4241), which is the point — a log that can be read and not found + is the half-surface that issue was about. + + Asked for every note, not only tasks. `upsert_note_embedding` is handed a + note_id and no kind — and the three writers that call it would each have to + learn to pass one — so a non-task simply has no rows and gets []. One + indexed lookup beside an ONNX forward pass over every chunk is not the + expense worth adding a parameter to three call sites for. + + Returns [] on any failure. A task whose logs could not be read should embed + as its own prose rather than not embed at all: less findable is recoverable + at the next write, unindexed is not. + """ + try: + async with async_session() as session: + result = await session.execute( + select(TaskLog.created_at, TaskLog.content) + .where(TaskLog.task_id == note_id) + .order_by(TaskLog.created_at.asc(), TaskLog.id.asc()) + ) + return list(result.all()) + except Exception: + logger.warning( + "Could not read work logs for note %d; embedding its own prose only", + note_id, exc_info=True, + ) + return [] + + async def upsert_note_embedding( note_id: int, user_id: int, title: str | None, body: str | None ) -> None: @@ -496,6 +547,7 @@ async def upsert_note_embedding( inserted in one transaction, so a concurrent read sees the old shape or the new one, never a mixture. """ + title, body = task_document(title, body, await _work_log_sections(note_id)) chunks = chunk_document(title, body) try: if not chunks: @@ -908,6 +960,71 @@ async def backfill_note_embeddings() -> None: # ── Rules (milestone 307, note 3026) ──────────────────────────────────── +# The heading a work log gets inside its task's embedded document. A CONSTANT +# because it is load-bearing twice over: `_split_sections` splits on it, so it +# is what keeps a log from being merged into the task's own prose, and it is +# what a reader sees at the top of a matched passage — "this is a log entry, +# not the task's description". Changing it changes the chunk boundaries of +# every task that has one, which is a CHUNKER_VERSION move. +WORK_LOG_HEADING = "## Work log" + + +def task_document( + title: str | None, + body: str | None, + logs: "Sequence[tuple[object, str | None]]" = (), +) -> tuple[str | None, str | None]: + """The (title, body) a TASK is EMBEDDED as — its prose plus its work logs. + + A synthesised embed-time shape, like `rule_document` and unlike a lesson: + the stored record is the task row, and the logs live in their own table, so + the document that should be searchable exists nowhere until it is built + here (#4251). + + WHY THE LOGS BELONG IN THE TASK'S DOCUMENT rather than in rows of their + own. "Has anyone tried this before?" is answered by a log and asked of a + task — a hit on a bare log would have to be resolved back to its task to be + worth anything, so the useful result is the task either way. The objection + to folding them in is that a long log drowns a short title, and that was + true before #280: one vector per record meant a 2,000-word log averaged the + task's own subject away, and everything past ~400 words was truncated + unread. Chunking removed both. Each log becomes its own section, each + section its own title-anchored vector, each scored separately — so a task + is as findable as its best-matching log, and the task's own prose keeps the + chunk it always had. + + That the result is LEGIBLE is the other half, and it is this session's + other build: a search now hands back the chunk that won (#4243), so a hit + earned by a log shows that log's passage under the task's title. Without it + the reader would get `body[:240]` of the task — the opening of a record + whose relevance lives three hundred lines further down. + + Ordering is oldest-first, matching how the web renders the narrative. Only + the heading date distinguishes the sections, so it is part of the shape: + "when was this tried" is half of what a log answers. + + An entry with no content is skipped rather than emitted as a bare heading — + an empty section is a vector with nothing in it but the task's title, which + competes with the task's real chunk and says nothing. + """ + sections = [] + for created_at, content in logs: + text = (content or "").strip() + if not text: + continue + stamp = getattr(created_at, "date", None) + heading = ( + f"{WORK_LOG_HEADING} — {stamp()}" if callable(stamp) + else WORK_LOG_HEADING + ) + sections.append(f"{heading}\n\n{text}") + if not sections: + return title, body + prose = (body or "").strip() + joined = "\n\n".join(sections) + return title, f"{prose}\n\n{joined}" if prose else joined + + def rule_document( title: str | None, statement: str | None, when_to_apply: str | None, ) -> tuple[str | None, str | None]: diff --git a/src/scribe/services/task_logs.py b/src/scribe/services/task_logs.py index e06975f..61853aa 100644 --- a/src/scribe/services/task_logs.py +++ b/src/scribe/services/task_logs.py @@ -14,6 +14,36 @@ logger = logging.getLogger(__name__) _UNSET = object() +async def _refresh_task_document(session, task_id: int) -> None: + """Re-embed the task whose work log just changed (#4251). + + A task's embedded document carries its logs, so a log written and not + indexed is #4241's half-surface wearing different clothes: the entry is + readable and unfindable, and the next session rebuilds what this one ruled + out. Create, edit and delete all go through here — an edited log that keeps + matching its old wording is the stale-vector problem `upsert_note_embedding` + already refuses to leave behind for a body. + + Loads the NOTE rather than synthesising one. `embed_note` reads + `title`, `body` and the OWNER's `user_id` off what it is handed, so a + stand-in row would index the logs under an empty title — throwing away the + per-chunk topical anchor that makes any of this discriminative — and file + the vectors under the wrong user. + + Failure is swallowed the way `embed_note`'s own is: a log that saved must + not fail on its index refresh. The startup backfill is the backstop. + """ + from scribe.services.notes import embed_note + + try: + result = await session.execute(select(Note).where(Note.id == task_id)) + note = result.scalars().first() + if note is not None: + embed_note(note) + except Exception: # noqa: BLE001 - indexing never breaks a write + logger.exception("embedding refresh failed for task %s", task_id) + + async def create_log( user_id: int, task_id: int, @@ -36,6 +66,7 @@ async def create_log( session.add(log) await session.commit() await session.refresh(log) + await _refresh_task_document(session, task_id) return log @@ -144,6 +175,7 @@ async def update_log( log.updated_at = datetime.now(timezone.utc) await session.commit() await session.refresh(log) + await _refresh_task_document(session, log.task_id) return log @@ -155,6 +187,8 @@ async def delete_log(user_id: int, log_id: int) -> bool: log = result.scalars().first() if log is None: return False + task_id = log.task_id await session.delete(log) await session.commit() + await _refresh_task_document(session, task_id) return True diff --git a/tests/helpers.py b/tests/helpers.py index d1fe6f9..8bc80f1 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -89,6 +89,24 @@ def make_mock_session() -> AsyncMock: return s +def session_returning(note): + """``make_mock_session`` whose every ``execute()`` yields `note` (or None). + + The commonest shape above the bare session: a service that loads one + record by id and acts on it. Two files had spelled this out identically + before a third was about to (#3207 — derive before the third copy), and + it belongs beside `make_mock_session` for the same reason that one does. + + For a service making SEVERAL different reads, set + ``session.execute.side_effect`` to a list of results instead. + """ + session = make_mock_session() + result = MagicMock() + result.scalars.return_value.first.return_value = note + session.execute = AsyncMock(return_value=result) + return session + + async def ensure_user(session, username: str, role: str = "user"): """Get-or-create a User by username inside an open session (flushed, not committed). diff --git a/tests/test_chunking.py b/tests/test_chunking.py index 3c56b01..618ec96 100644 --- a/tests/test_chunking.py +++ b/tests/test_chunking.py @@ -185,11 +185,17 @@ async def test_search_collapses_chunk_rows_to_best_chunk_per_note(): # --- the write path: one row per chunk (#280 step 3) ------------------------- -def _session_ctx(): +def _session_ctx(log_rows=()): + """A session stand-in. `log_rows` answers the work-log read a task's + document now needs (#4251) — answered explicitly rather than left to + autovivify, because `list(result.all())` on a bare MagicMock raises and is + swallowed, which would quietly make every one of these a no-log test.""" from unittest.mock import AsyncMock, MagicMock session = MagicMock() - session.execute = AsyncMock() + result = MagicMock() + result.all.return_value = list(log_rows) + session.execute = AsyncMock(return_value=result) session.commit = AsyncMock() ctx = MagicMock() ctx.__aenter__ = AsyncMock(return_value=session) @@ -226,6 +232,32 @@ async def test_upsert_stores_one_versioned_row_per_chunk(): session.execute.assert_awaited() # the delete that makes replacement atomic +async def test_a_tasks_work_logs_reach_the_rows_it_is_stored_as(): + """The wiring half of #4251: the shaper is pure and tested next door, so + what is pinned here is that the WRITER actually asks for the logs and + embeds what comes back. A log that is written, readable (#4241) and absent + from the index is the same half-surface one layer down.""" + import datetime + from unittest.mock import AsyncMock, patch + + from scribe.services import embeddings as emb + + session, ctx = _session_ctx( + log_rows=[(datetime.datetime(2026, 9, 20), "ruled out the cache theory")] + ) + with ( + patch.object(emb, "async_session", return_value=ctx), + patch.object(emb, "get_embeddings", AsyncMock(return_value=[[0.0] * 384])), + ): + await emb.upsert_note_embedding(7, 42, "A task", "Its own prose.") + + rows = [call.args[0] for call in session.add.call_args_list] + stored = "\n".join(r.chunk_text for r in rows) + assert "ruled out the cache theory" in stored + assert "Its own prose." in stored + assert emb.WORK_LOG_HEADING in stored + + async def test_upsert_of_an_emptied_record_clears_rows_instead_of_embedding(): """An empty embedding is worse than none, and a STALE one is worse than that — a record emptied of content must stop being findable by what it no diff --git a/tests/test_embedding_yields_to_a_delete.py b/tests/test_embedding_yields_to_a_delete.py index ba50388..f4c2199 100644 --- a/tests/test_embedding_yields_to_a_delete.py +++ b/tests/test_embedding_yields_to_a_delete.py @@ -32,6 +32,11 @@ def _mock_session(lock_result: object = 7, execute_side_effect=None): session = MagicMock() claimed = MagicMock() claimed.scalar_one_or_none.return_value = lock_result + # A task's document carries its work logs (#4251), so the refresh reads + # task_logs before it builds chunks. Answered explicitly — left to + # autovivify, `list(result.all())` would raise and be swallowed, and these + # tests would be exercising the failure path without saying so. + claimed.all.return_value = [] if execute_side_effect is not None: session.execute = AsyncMock(side_effect=execute_side_effect) else: @@ -65,10 +70,19 @@ async def test_a_note_refresh_claims_the_row_before_rewriting_its_vectors(): ): await emb.upsert_note_embedding(7, 42, "T", "a short body") - claim, replace = [c.args[0] for c in session.execute.call_args_list][:2] - assert compiled_sql(claim, dialect=PG).startswith("SELECT notes.id") - assert "FOR KEY SHARE NOWAIT" in compiled_sql(claim, dialect=PG) - assert compiled_sql(replace, dialect=PG).startswith("DELETE FROM note_embeddings") + sqls = [compiled_sql(c.args[0], dialect=PG) for c in session.execute.call_args_list] + # Located by what they ARE rather than by position: the work-log read a + # task's document needs (#4251) runs before any of this, and an index is + # not what the claim is about. + claim = next(i for i, sql in enumerate(sqls) if "FOR KEY SHARE NOWAIT" in sql) + replace = next( + i for i, sql in enumerate(sqls) if sql.startswith("DELETE FROM note_embeddings") + ) + assert sqls[claim].startswith("SELECT notes.id") + assert claim < replace, "the claim goes first — that is the whole fix" + assert not any("note_embeddings" in sql for sql in sqls[:claim]), ( + "nothing may touch the chunk rows before the parent is claimed" + ) session.add.assert_called() @@ -113,6 +127,8 @@ async def test_a_record_already_gone_is_not_re_embedded(): ): await emb.upsert_note_embedding(7, 42, "T", "a short body") - assert session.execute.await_count == 1 + sqls = [compiled_sql(c.args[0], dialect=PG) for c in session.execute.call_args_list] + assert any("FOR KEY SHARE NOWAIT" in sql for sql in sqls), "it did reach the claim" + assert not any("note_embeddings" in sql for sql in sqls), "and stopped there" session.add.assert_not_called() session.commit.assert_not_awaited() diff --git a/tests/test_services_dedup.py b/tests/test_services_dedup.py index 005f1ac..f7c2b33 100644 --- a/tests/test_services_dedup.py +++ b/tests/test_services_dedup.py @@ -14,23 +14,14 @@ from scribe.services.dedup import ( plan_candidate_text, plan_match_response, ) -from tests.helpers import fake_note, make_mock_session - - -def _session_returning(note): - """A mocked async_session() whose single execute() yields `note` (or None).""" - s = make_mock_session() - result = MagicMock() - result.scalars.return_value.first.return_value = note - s.execute = AsyncMock(return_value=result) - return s +from tests.helpers import fake_note, make_mock_session, session_returning @pytest.mark.asyncio async def test_title_exact_match_returns_title_duplicate(): note = fake_note(id=10, title="Setup CI") with patch("scribe.services.dedup.async_session", - return_value=_session_returning(note)): + return_value=session_returning(note)): # whitespace/case differences are normalized away dup = await find_duplicate_note(7, " setup ci ", project_id=2, is_task=True) assert dup is not None @@ -43,7 +34,7 @@ async def test_title_exact_match_returns_title_duplicate(): async def test_short_body_skips_semantic_check(): sem = AsyncMock() with patch("scribe.services.dedup.async_session", - return_value=_session_returning(None)), \ + return_value=session_returning(None)), \ patch("scribe.services.dedup.embeddings_svc.semantic_search_notes", sem): dup = await find_duplicate_note(7, "Unique", body="too short", project_id=2) assert dup is None @@ -55,7 +46,7 @@ async def test_semantic_match_when_body_substantial(): hit = fake_note(id=20, title="Existing", note_type="note") sem = AsyncMock(return_value=[(0.93, hit)]) with patch("scribe.services.dedup.async_session", - return_value=_session_returning(None)), \ + return_value=session_returning(None)), \ patch("scribe.services.dedup.embeddings_svc.semantic_search_notes", sem): dup = await find_duplicate_note( 7, "Title", body="x" * 250, project_id=2, is_task=False, note_type="note", @@ -84,7 +75,7 @@ async def test_gate_catches_a_duplicate_hiding_in_a_later_chunk(): # Every chunk misses except the LAST one the gate will ask about. sem = AsyncMock(side_effect=[[] for _ in range(n_chunks - 1)] + [[(0.94, hit)]]) with patch("scribe.services.dedup.async_session", - return_value=_session_returning(None)), \ + return_value=session_returning(None)), \ patch("scribe.services.dedup.embeddings_svc.semantic_search_notes", sem): dup = await find_duplicate_note( 7, "Title", body=body, project_id=2, is_task=False, note_type="note", @@ -98,7 +89,7 @@ async def test_semantic_match_of_other_note_type_is_ignored(): other = fake_note(id=21, title="X", note_type="process") sem = AsyncMock(return_value=[(0.97, other)]) with patch("scribe.services.dedup.async_session", - return_value=_session_returning(None)), \ + return_value=session_returning(None)), \ patch("scribe.services.dedup.embeddings_svc.semantic_search_notes", sem): dup = await find_duplicate_note(7, "Title", body="x" * 250, note_type="note") assert dup is None # type mismatch must not block @@ -108,7 +99,7 @@ async def test_semantic_match_of_other_note_type_is_ignored(): async def test_rule_title_match_in_topic(): rule = fake_note(id=47, title="Honor the multi-user sharing ACL") with patch("scribe.services.dedup.async_session", - return_value=_session_returning(rule)): + return_value=session_returning(rule)): dup = await find_duplicate_rule( "honor the multi-user sharing acl", topic_id=7, ) @@ -361,7 +352,7 @@ async def test_plan_title_match_short_circuits_the_semantic_arm(): ms = MagicMock(id=415, title="Plan gate") sem = AsyncMock() with patch("scribe.services.dedup.can_read_project", AsyncMock(return_value=True)), \ - patch("scribe.services.dedup.async_session", return_value=_session_returning(ms)), \ + patch("scribe.services.dedup.async_session", return_value=session_returning(ms)), \ patch("scribe.services.dedup.embeddings_svc.semantic_search_milestones", sem): dup = await find_matching_plan(7, 2, " plan GATE", "x" * 300) assert (dup.id, dup.reason, dup.similarity) == (415, "title", 1.0) @@ -373,7 +364,7 @@ async def test_plan_semantic_arm_asks_for_active_plans_in_the_project_at_the_set ms = MagicMock(id=9, title="Metadata") sem = AsyncMock(return_value=[(0.912345, ms)]) with patch("scribe.services.dedup.can_read_project", AsyncMock(return_value=True)), \ - patch("scribe.services.dedup.async_session", return_value=_session_returning(None)), \ + patch("scribe.services.dedup.async_session", return_value=session_returning(None)), \ patch("scribe.services.dedup.embeddings_svc.semantic_search_milestones", sem), \ patch("scribe.services.settings.get_setting", AsyncMock(return_value="0.8")): dup = await find_matching_plan(7, 2, "Book metadata", "x" * 300) @@ -396,7 +387,7 @@ async def test_plan_gate_fails_open(): @pytest.mark.asyncio async def test_plan_gate_says_nothing_about_a_project_the_caller_cannot_read(): ms = MagicMock(id=415, title="Their plan") - session = MagicMock(return_value=_session_returning(ms)) + session = MagicMock(return_value=session_returning(ms)) with patch("scribe.services.dedup.can_read_project", AsyncMock(return_value=False)), \ patch("scribe.services.dedup.async_session", session): assert await find_matching_plan(8, 2, "Their plan") is None diff --git a/tests/test_task_document_shape.py b/tests/test_task_document_shape.py new file mode 100644 index 0000000..92dc9f0 --- /dev/null +++ b/tests/test_task_document_shape.py @@ -0,0 +1,200 @@ +"""task_document — the document a TASK is embedded as, work logs included (#4251). + +A work log is the richest record Scribe holds of *why* something is the way it +is: prose written during the work, saying what was tried and ruled out. #4241 +made it readable. It was still not findable, so "has anyone tried this +approach?" — the question a log answers — could not reach one, and #4208 was +rebuilt in this very session because its logs were unreachable. + +These tests pin the shape and the two properties the decision rested on: a task +without logs embeds EXACTLY as it always did, and a task with them stays as +discriminative as it was, because each log is its own title-anchored chunk +rather than prose averaged into one vector. +""" +import datetime + +from scribe.services.embeddings import ( + WORK_LOG_HEADING, + chunk_document, + embedding_text, + task_document, +) + +D1 = datetime.datetime(2026, 9, 20, 10, 0) +D2 = datetime.datetime(2026, 9, 21, 11, 0) + + +# --- the identity half: a task with no logs is untouched --------------------- + + +def test_a_task_with_no_logs_embeds_exactly_as_it_did_before(): + """Most notes are not tasks and most tasks carry no log. Their vectors are + the corpus this instance's thresholds were measured against, and changing + them for nothing would move every tuned number underneath itself (#4225).""" + assert task_document("A title", "A body") == ("A title", "A body") + assert task_document("A title", "A body", []) == ("A title", "A body") + assert task_document("T", None) == ("T", None) + + +def test_an_entry_with_no_content_is_skipped_rather_than_emitted_empty(): + """A bare heading is a vector containing nothing but the task's title — it + would compete with the task's real chunk and say nothing.""" + _t, body = task_document("T", "prose", [(D1, " "), (D2, None), (D1, "real")]) + assert body.count(WORK_LOG_HEADING) == 1 + assert "real" in body + + +def test_a_task_that_is_only_logs_still_yields_a_document(): + """A task opened with a title and filled in entirely through its log — the + shape every step of a milestone starts as.""" + title, body = task_document("T", "", [(D1, "the only content")]) + assert title == "T" + assert body.startswith(WORK_LOG_HEADING) + assert "the only content" in body + + +# --- the shape: the logs are sections, oldest first -------------------------- + + +def test_logs_are_appended_as_dated_sections_after_the_tasks_own_prose(): + title, body = task_document( + "Fix the thing", "The task prose.", + [(D1, "Tried X, ruled out."), (D2, "Y worked.")], + ) + assert title == "Fix the thing" + assert body.index("The task prose.") < body.index("Tried X") < body.index("Y worked.") + assert f"{WORK_LOG_HEADING} — 2026-09-20" in body + assert f"{WORK_LOG_HEADING} — 2026-09-21" in body + + +def test_an_entry_with_no_timestamp_still_gets_its_own_section(): + """Degrades to an undated heading rather than dropping the entry or + crashing — a restored row or a hand-built one is not a reason to lose the + richest prose in the record.""" + _t, body = task_document("T", "p", [(None, "content from nowhere")]) + assert WORK_LOG_HEADING in body + assert "content from nowhere" in body + + +# --- why folding them in is safe: chunking, not averaging -------------------- + + +def test_each_log_becomes_its_own_title_anchored_chunk(): + """THE decision this build turns on. The objection to putting logs in the + task's document is that a long log drowns a short title — true before #280, + when one vector per record meant a 2,000-word log averaged the task's + subject away and everything past ~400 words was truncated unread. Chunking + answers both: separate vectors, each carrying the title as its anchor.""" + def _entry(subject: str) -> str: + para = " ".join([f"This log entry discusses the {subject} in detail."] * 8) + return "\n\n".join(f"{para} (p{i})" for i in range(6)) + + title, body = task_document( + "Short task title", "Short body.", + [(D1, _entry("approach")), (D2, _entry("alternative"))], + ) + chunks = chunk_document(title, body) + assert len(chunks) > 1, "a long log must split rather than truncate" + for chunk in chunks: + assert chunk.startswith("Short task title\n") + + # The task's own prose still has a chunk of its own — it is not merged into + # a log section and scored as part of it. + assert any("Short body." in c and "alternative" not in c for c in chunks) + + # And nothing is lost: this is what #280 exists for. Paragraph-shaped, + # because that is what a log is and because a single unbroken 3,000-char + # line is hard-split mid-line by design — `test_chunking` owns that case. + joined = "\n".join(chunks) + for line in body.splitlines(): + if line.strip(): + assert line.strip() in joined, f"content dropped: {line[:60]!r}" + + +def test_a_short_task_with_a_short_log_is_still_one_sharp_chunk(): + """Over-splitting is not the goal either. A task and one short log stay a + single document, the shape note #2485 measured as the sharpest.""" + title, body = task_document("T", "body", [(D1, "a short note about it")]) + assert chunk_document(title, body) == [embedding_text(title, body)] + + +# --- keeping it current: a log write refreshes the task's vectors ------------ +# +# The shaper above is pure. These pin that the writers actually call it — a +# task whose logs are in the document but never re-indexed after one is written +# is #4241's half-surface one layer down: the entry is readable, and the search +# still answers as though it were never written. + +from unittest.mock import MagicMock, patch # noqa: E402 + +import pytest # noqa: E402 + +from scribe.services import task_logs as svc # noqa: E402 +from tests.helpers import ( # noqa: E402 + fake_note, make_mock_session, session_returning, +) + + +@pytest.mark.asyncio +async def test_writing_a_work_log_refreshes_the_tasks_embedding(): + note = fake_note(id=7, title="A task", body="prose", is_task=True) + session = session_returning(note) + with ( + patch.object(svc, "async_session", return_value=session), + patch("scribe.services.notes.embed_note") as embed, + ): + await svc.create_log(42, 7, "what I tried") + embed.assert_called_once() + assert embed.call_args.args[0] is note + + +@pytest.mark.asyncio +async def test_editing_a_work_log_refreshes_it_too(): + """An edited log whose vectors still carry the old wording keeps matching + what it no longer says — the stale-vector case `upsert_note_embedding` + already refuses to leave behind for a body.""" + note = fake_note(id=7, title="A task", body="prose", is_task=True) + log = MagicMock(id=3, task_id=7) + session = make_mock_session() + found_log, found_note = MagicMock(), MagicMock() + found_log.scalars.return_value.first.return_value = log + found_note.scalars.return_value.first.return_value = note + session.execute.side_effect = [found_log, found_note] + with ( + patch.object(svc, "async_session", return_value=session), + patch("scribe.services.notes.embed_note") as embed, + ): + await svc.update_log(42, 3, content="reworded") + embed.assert_called_once_with(note) + + +@pytest.mark.asyncio +async def test_deleting_a_work_log_refreshes_it_too(): + """And reads the task id BEFORE the row goes — after the delete there is + nothing left to ask which task it belonged to.""" + note = fake_note(id=7, title="A task", body="prose", is_task=True) + log = MagicMock(id=3, task_id=7) + session = make_mock_session() + found_log, found_note = MagicMock(), MagicMock() + found_log.scalars.return_value.first.return_value = log + found_note.scalars.return_value.first.return_value = note + session.execute.side_effect = [found_log, found_note] + with ( + patch.object(svc, "async_session", return_value=session), + patch("scribe.services.notes.embed_note") as embed, + ): + assert await svc.delete_log(42, 3) is True + embed.assert_called_once_with(note) + + +@pytest.mark.asyncio +async def test_a_failed_refresh_does_not_fail_the_log_that_saved(): + """Indexing never breaks a write. The startup backfill is the backstop.""" + session = session_returning(fake_note(id=7, title="T", body="b", is_task=True)) + with ( + patch.object(svc, "async_session", return_value=session), + patch("scribe.services.notes.embed_note", side_effect=RuntimeError("boom")), + ): + log = await svc.create_log(42, 7, "what I tried") + assert log is not None + session.commit.assert_awaited() -- 2.54.0 From 1fca8c2808fc7bea9970114ab249c7f664eed03b Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 11:25:53 -0400 Subject: [PATCH 9/9] feat(retrieval): a System's charter becomes an answer, not just a filter (#4251) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 2 of #4251. A System's `description` is a charter — several hundred words saying what belongs in that area and what does not — and it is the answer to "which part of this codebase does X live in". There was no semantic path to one: `list_systems` enumerates, and `search(system_id=…)` uses a System as a FILTER over notes. So a System could narrow a search and could never be the answer to one, and an agent asking where a record belonged had to read every charter or guess. ITS OWN SEARCH, not a `content_type` over notes, for the reason note 3163 gives about milestones: the row could be shared, the search cannot. A charter competing with the whole note corpus for one top-k is outranked by the records filed under it — the right answer crowded out by its own contents — and "where does this belong?" is a different question from "what prior art is there?", which a caller asking one should not have to read past answers to. So `system_embeddings` (0107) joins note_, rule_ and milestone_embeddings as the fourth sibling, with `system_document`, `upsert_system_embedding`, `semantic_search_systems`, a startup backfill and `search(content_type= "system")`. Scoped like milestones: with a project_id, that project's Systems if the caller can read the project (rule 78); without one, the caller's own. Archived Systems are excluded — an archived area is one the operator has said is no longer where things go, which is exactly the question being asked. `system_document` is the plainest of the four shapes on purpose. A charter is already written as the thing this search has to match, in the words someone asking would use — so there is no trigger to synthesise as `rule_document` must, and no second record to gather as `task_document` must. The stored charter IS the sharp document, the way a snippet's is. `color`, `status` and `order_index` stay out: presentation and bookkeeping, and a vector carrying them would be answering a question nobody asks of a charter. The search publishes `report["best_chunk"]` from the start rather than being retrofitted, which is what #4251 asked of any fourth search. It matters more here than anywhere: a charter runs long and a result shows its NAME, so a match on the paragraph that actually decides where a record belongs would otherwise be previewed by two words that cannot say. The id that comes back is the one `system_id`, `system_ids` and `list_system_records` already take, so the answer to "where does this belong?" is directly usable as "show me what is there" and as "file it here". `embed_system` sits beside `notes.embed_note` at the service for #2056's reason — every door gets it by construction. Not called on delete: that is a soft delete and the search joins through `System`, so the vectors are already unreachable, and leaving them means a restore is findable again immediately. `system_embeddings` is declared in backup's `_NOT_INCLUDED` as derived, beside its three siblings. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- alembic/versions/0107_system_embeddings.py | 67 +++++++ src/scribe/app.py | 10 +- src/scribe/mcp/tools/search.py | 63 ++++++- src/scribe/models/__init__.py | 4 +- src/scribe/models/embedding.py | 39 ++++ src/scribe/services/backup.py | 3 +- src/scribe/services/embeddings.py | 206 +++++++++++++++++++++ src/scribe/services/systems.py | 34 ++++ tests/test_mcp_tool_search.py | 82 +++++++- tests/test_system_charter_search.py | 164 ++++++++++++++++ 10 files changed, 663 insertions(+), 9 deletions(-) create mode 100644 alembic/versions/0107_system_embeddings.py create mode 100644 tests/test_system_charter_search.py diff --git a/alembic/versions/0107_system_embeddings.py b/alembic/versions/0107_system_embeddings.py new file mode 100644 index 0000000..e98251c --- /dev/null +++ b/alembic/versions/0107_system_embeddings.py @@ -0,0 +1,67 @@ +"""system_embeddings — a charter becomes an ANSWER, not just a filter (#4251) + +Revision ID: 0107 +Revises: 0106 +Create Date: 2026-09-21 + +A System's `description` is a charter: several hundred words saying what +belongs in that area and what does not. It is the answer to "which part of +this codebase does X live in", and there was no semantic path to it. +`list_systems` enumerates and `search(system_id=…)` uses a System as a FILTER +over notes — so a System could narrow a search and could never be the answer +to one, and an agent asking where a record belonged had to read every charter +or guess. + +The fourth sibling of note_embeddings (0067), rule_embeddings (0089) and +milestone_embeddings (0102), and for the same reason note 3163 gives about the +third: the row could be shared, the search cannot. "Where does this belong?" +is a different question from "what prior art is there?", and no note or rule +search can answer it, because a charter is not a note. + +The vectors are DERIVED: nothing is backfilled here, the startup backfill +writes them. +""" +import sqlalchemy as sa +from alembic import op + +revision = "0107" +down_revision = "0106" +branch_labels = None +depends_on = None + +# Matches its three siblings — bge-small-en-v1.5, 384-dim. +_EMBEDDING_DIM = 384 + + +def upgrade() -> None: + op.create_table( + "system_embeddings", + sa.Column( + "system_id", sa.Integer(), + sa.ForeignKey("systems.id", ondelete="CASCADE"), primary_key=True, + ), + sa.Column("chunk_index", sa.Integer(), primary_key=True), + sa.Column("chunk_text", sa.Text(), nullable=False), + sa.Column("chunker_version", sa.Integer(), nullable=False), + sa.Column( + "updated_at", sa.DateTime(timezone=True), nullable=False, + server_default=sa.text("now()"), + ), + ) + # Raw DDL for the vector column, as 0067, 0089 and 0102 do: the type comes + # from the pgvector extension, not SQLAlchemy's type system. + op.execute( + f"ALTER TABLE system_embeddings ADD COLUMN embedding vector({_EMBEDDING_DIM}) NOT NULL" + ) + op.execute( + """ + CREATE INDEX ix_system_embeddings_embedding_hnsw + ON system_embeddings + USING hnsw (embedding vector_cosine_ops) + """ + ) + + +def downgrade() -> None: + op.execute("DROP INDEX IF EXISTS ix_system_embeddings_embedding_hnsw") + op.drop_table("system_embeddings") diff --git a/src/scribe/app.py b/src/scribe/app.py index a0e3362..0ebfa7d 100644 --- a/src/scribe/app.py +++ b/src/scribe/app.py @@ -173,7 +173,8 @@ def create_app() -> Quart: from scribe.services.auth import start_auth_token_retention_loop from scribe.services.embeddings import ( - backfill_milestone_embeddings, backfill_note_embeddings, backfill_rule_embeddings, + backfill_milestone_embeddings, backfill_note_embeddings, + backfill_rule_embeddings, backfill_system_embeddings, ) from scribe.services.logging import start_log_retention_loop from scribe.services.notifications import start_notification_loop @@ -243,6 +244,13 @@ def create_app() -> Quart: await backfill_milestone_embeddings() except Exception: logger.warning("Milestone embedding backfill failed", exc_info=True) + # Systems got vectors in #4251, so before this pass every charter + # ever written is unfindable — a System could narrow a search and + # never be the answer to one. + try: + await backfill_system_embeddings() + except Exception: + logger.warning("System embedding backfill failed", exc_info=True) # Snippets written before migration 0070 have no `notes.data` mirror, # and the location reverse lookup queries that column — an unfilled # row would read as "no snippet here" rather than as a gap. Separate diff --git a/src/scribe/mcp/tools/search.py b/src/scribe/mcp/tools/search.py index 6f05632..2ffc329 100644 --- a/src/scribe/mcp/tools/search.py +++ b/src/scribe/mcp/tools/search.py @@ -15,7 +15,7 @@ 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, - semantic_search_rules, + semantic_search_rules, semantic_search_systems, ) from scribe.services import rulebooks as rulebooks_svc from scribe.services.retrieval_telemetry import record_retrieval, retrieval_summary @@ -38,7 +38,7 @@ _EXCERPT_CHARS = 1000 # 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") +_OWN_SEARCH = ("rule", "milestone", "system") async def _search_rules(uid: int, q: str, limit: int, project_id: int) -> dict: @@ -141,6 +141,53 @@ async def _search_milestones(uid: int, q: str, limit: int, project_id: int) -> d } +async def _search_systems(uid: int, q: str, limit: int, project_id: int) -> dict: + """Systems by meaning — "where does this belong?" (#4251). + + A System's `description` is a charter: several hundred words saying what + belongs in that area and what does not. `list_systems` enumerates them and + `system_id` filters by one, so before this a System could NARROW a search + and could never be the answer to one — an agent asking where a record + belonged had to read every charter or guess. + + Its own result shape and its own search, not a `content_type` over notes, + because the question is different: "where does this belong?" is not "what + prior art is there?". A charter competing with the whole note corpus for + one top-k would also be outranked by the records filed under it, and the + right answer would be crowded out by its own contents. + + The charter's `matched` passage comes along rather than the whole thing. + A charter is long and the paragraph that decides where a record belongs is + the one worth reading; the rest is get_system (#4243). + + The id that comes back is the one `search(system_id=…)`, + `list_system_records` and every `system_ids` argument take — so the answer + to "where does this belong?" is directly usable as "show me what is there" + and as "file it here". + """ + report: dict = {} + raw = await semantic_search_systems( + uid, q, project_id=project_id or None, limit=limit, report=report, + ) + chunks = report.get("best_chunk") or {} + return { + "results": [ + { + "id": sys_.id, + "name": sys_.name, + **excerpt_fields( + sys_.description or "", chunks.get(int(sys_.id)), + _EXCERPT_CHARS, key="matched", + ), + "project_id": sys_.project_id, + "similarity": float(score), + } + for score, sys_ in raw + ], + "total": len(raw), + } + + def result_excerpt(note, chunk: dict | None) -> dict: """The part of a record a caller judges "should I open this?" on. @@ -204,8 +251,14 @@ async def search( 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). + THREE KINDS WITH THEIR OWN SEARCH AND THEIR OWN RESULT SHAPE, + because each answers a question no note search can: + 'rule' (the operator's standing instructions — searchable by + meaning since milestone 307), 'milestone' ("is there already a + plan for this?"), and 'system' ("where does this belong?" — a + System's charter says what belongs in an area and what does not, + and the id it returns is the one `system_id` and `system_ids` + take). Reach for 'rule' when you want to know whether a standing instruction covers something: "is there a rule about release tagging?". A hit carries the rule's `why` and `how_to_apply`, @@ -258,6 +311,8 @@ 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) + if content_type == "system": + return await _search_systems(uid, q, limit, project_id) filters = content_type_filters(content_type, extra=_OWN_SEARCH) is_task = filters.get("is_task") t0 = time.perf_counter() diff --git a/src/scribe/models/__init__.py b/src/scribe/models/__init__.py index 1584b5f..abdcec7 100644 --- a/src/scribe/models/__init__.py +++ b/src/scribe/models/__init__.py @@ -53,7 +53,9 @@ from scribe.models.user import User # noqa: E402, F401 from scribe.models.app_log import AppLog # noqa: E402, F401 from scribe.models.password_reset import PasswordResetToken # noqa: E402, F401 from scribe.models.invitation import InvitationToken # noqa: E402, F401 -from scribe.models.embedding import MilestoneEmbedding, NoteEmbedding, RuleEmbedding # noqa: E402, F401 +from scribe.models.embedding import ( # noqa: E402, F401 + MilestoneEmbedding, NoteEmbedding, RuleEmbedding, SystemEmbedding, +) from scribe.models.retrieval_log import RetrievalLog # noqa: E402, F401 from scribe.models.retrieval_tuning import RetrievalTuningEvent # noqa: E402, F401 from scribe.models.note_usage import NoteUsageEvent # noqa: E402, F401 diff --git a/src/scribe/models/embedding.py b/src/scribe/models/embedding.py index 3ab02a2..de5797c 100644 --- a/src/scribe/models/embedding.py +++ b/src/scribe/models/embedding.py @@ -128,3 +128,42 @@ class MilestoneEmbedding(Base): DateTime(timezone=True), default=lambda: datetime.now(timezone.utc), ) + + +class SystemEmbedding(Base): + """One embedding vector per CHUNK of a System's charter (#4251). + + The fourth sibling, for the reason note 3163 gives about the third: the row + could be shared, the search cannot. A System's `description` is a charter — + several hundred words saying what belongs in that area and what does not — + and it is the answer to "which part of this codebase does X live in". + Before this there was no semantic path to one: `list_systems` enumerates, + and `search(system_id=…)` uses a System as a FILTER over notes. So a System + could narrow a search and could never be the answer to one. + + "Where does this belong?" is a different question from "what prior art is + there?", which is why this is its own search rather than a note_type: a + charter competing with two thousand notes for the same top-k would be + outranked by the records filed under it, and the right answer would be + crowded out by its own contents. + + The document is the name and the charter. Derived data: the startup + backfill regenerates it, which is also how a chunker-version bump is + handled. + """ + + __tablename__ = "system_embeddings" + + system_id: Mapped[int] = mapped_column( + Integer, + ForeignKey("systems.id", ondelete="CASCADE"), + primary_key=True, + ) + chunk_index: Mapped[int] = mapped_column(Integer, primary_key=True) + embedding: Mapped[list] = mapped_column(Vector(EMBEDDING_DIM), nullable=False) + chunk_text: Mapped[str] = mapped_column(Text, nullable=False) + chunker_version: Mapped[int] = mapped_column(Integer, nullable=False) + updated_at: Mapped[datetime] = mapped_column( + DateTime(timezone=True), + default=lambda: datetime.now(timezone.utc), + ) diff --git a/src/scribe/services/backup.py b/src/scribe/services/backup.py index 80bd84b..0f515fc 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -123,7 +123,7 @@ _BACKED_UP = [ # Tables intentionally NOT in the backup, surfaced in the payload so the gap is # explicit rather than silent. ACL (groups/shares) is a coherent follow-up; -# note_embeddings and rule_embeddings are derived (regenerated at startup +# the four *_embeddings tables are derived (regenerated at startup # from the records themselves, which is also how a chunker bump is handled); api_keys are # sensitive credentials; retrieval_logs is observational telemetry that nothing # reads for correctness and that grows per query; the rest are @@ -135,6 +135,7 @@ _BACKED_UP = [ _NOT_INCLUDED = [ "groups", "group_memberships", "project_shares", "note_shares", "api_keys", "note_embeddings", "rule_embeddings", "milestone_embeddings", + "system_embeddings", "app_logs", "notifications", "invitation_tokens", "password_reset_tokens", "user_profiles", "retrieval_logs", diff --git a/src/scribe/services/embeddings.py b/src/scribe/services/embeddings.py index 7927fd6..eba3c5f 100644 --- a/src/scribe/services/embeddings.py +++ b/src/scribe/services/embeddings.py @@ -29,6 +29,7 @@ from scribe.services.access import can_read_project, notes_visibility_clause if TYPE_CHECKING: # resolves forward refs without importing at runtime from scribe.models.milestone import Milestone from scribe.models.rulebook import Rule + from scribe.models.system import System logger = logging.getLogger(__name__) @@ -1451,6 +1452,211 @@ async def semantic_search_milestones( return kept +# --- Systems: the charter as an ANSWER, not a filter (#4251) ----------------- + + +def system_document( + name: str | None, description: str | None +) -> tuple[str | None, str | None]: + """The (title, body) a System is EMBEDDED as — its name and its charter. + + The plainest of the four document shapes, and deliberately so. A System's + `description` is already written as the thing this search has to match: it + says what belongs in the area and what does not, which is the answer to + "where does this go?" in the words someone asking would use. There is no + trigger to synthesise, as `rule_document` must, and no separate record to + gather in, as `task_document` must — the stored charter IS the sharp + document, the way a snippet's is. + + `color`, `status` and `order_index` are left out. They are presentation and + bookkeeping; a vector that carried them would be answering a question + nobody asks of a charter. + + A System with no charter yet degrades to its name. It still embeds, just + weakly — a bare name is exactly what the model docstring says is never + enough, and this is an argument for writing the charter, not for padding + the document with whatever is to hand. + """ + return (name or "").strip() or None, (description or "").strip() or None + + +async def upsert_system_embedding( + system_id: int, name: str | None, description: str | None +) -> None: + """Chunk, embed and persist a System's vectors. Safe to fire-and-forget. + + The third sibling's contract exactly: one document definition shared by the + write path and the backfill, and an atomic per-System replacement guarded + by the parent-row claim (#3262), so a System deleted mid-refresh wins. + """ + from scribe.models.embedding import SystemEmbedding + from scribe.models.system import System + + doc_title, doc_body = system_document(name, description) + chunks = chunk_document(doc_title, doc_body) + try: + if not chunks: + async with async_session() as session: + await session.execute( + delete(SystemEmbedding).where(SystemEmbedding.system_id == system_id) + ) + await session.commit() + return + except Exception: + logger.warning("Failed to clear embedding for system %d", system_id, exc_info=True) + return + + try: + vectors = await get_embeddings(chunks) + except Exception: + logger.debug("Skipping embedding for system %d — embedder unavailable", system_id) + return + + try: + async with async_session() as session: + if not await _claim_parent_row(session, System.id, system_id, "system"): + return + await session.execute( + delete(SystemEmbedding).where(SystemEmbedding.system_id == system_id) + ) + for index, (chunk, vector) in enumerate(zip(chunks, vectors)): + session.add(SystemEmbedding( + system_id=system_id, chunk_index=index, embedding=vector, + chunk_text=chunk, chunker_version=CHUNKER_VERSION, + )) + await session.commit() + except Exception: + logger.warning("Failed to persist embedding for system %d", system_id, exc_info=True) + + +async def semantic_search_systems( + user_id: int, + query: str, + *, + project_id: int | None = None, + limit: int = 5, + threshold: float = _SIMILARITY_THRESHOLD, + report: dict | None = None, +) -> list[tuple[float, "System"]]: + """Return up to *limit* (score, system) pairs most like *query*. + + Answers "where does this belong?" — the question asked before filing a + record or opening a file, and the one that had no tool: `list_systems` + enumerates and `system_id` filters, so a charter could narrow a search and + could never be the answer to one. + + ITS OWN SEARCH rather than a note kind, for what note 3163 says about + milestones: a charter competing with the whole note corpus for one top-k + would be outranked by the records filed under it, and the right answer + would be crowded out by its own contents. The questions are different too — + "where does this belong?" is not "what prior art is there?" — and a caller + asking one should not have to read past answers to the other. + + SCOPE. With `project_id`, that project's Systems, provided the caller can + read the project (access.can_read_project, rule 78) — a collaborator on a + shared project sees its areas, which is the point of a charter. Without + one, the Systems the caller owns across their projects. Archived Systems + are excluded: an archived area is one the operator has said is no longer + where things go, and that is exactly the question being asked. + + Collapses to best-chunk-per-System and publishes the winning chunk, like + the sibling searches — from the start rather than retrofitted (#4243). + Returns an empty list if the embedder is unavailable, the project is not + readable, or on any error: a recall aid must never break the call it + serves. + """ + from scribe.models.embedding import SystemEmbedding + from scribe.models.system import System + + if not query or not query.strip(): + return [] + try: + query_vec = await get_embedding(query) + except Exception: + logger.debug("System search skipped — embedder unavailable") + return [] + + distance = SystemEmbedding.embedding.cosine_distance(query_vec) + try: + if project_id: + if not await can_read_project(user_id, project_id): + return [] + scope = System.project_id == project_id + else: + scope = System.user_id == user_id + async with async_session() as session: + rows = (await session.execute( + select( + System, + distance.label("distance"), + SystemEmbedding.chunk_index, + SystemEmbedding.chunk_text, + ) + .select_from(SystemEmbedding) + .join(System, SystemEmbedding.system_id == System.id) + .where( + scope, + System.deleted_at.is_(None), + System.status != "archived", + ) + .order_by(distance) + .limit(limit * _CHUNK_OVERFETCH) + )).all() + except Exception: + logger.warning("System semantic search failed", exc_info=True) + return [] + + best: dict[int, tuple[float, object]] = {} + # A charter runs to several hundred words and a result shows its NAME — so + # a match on the paragraph that actually decides where a record belongs + # would be previewed by two words that cannot. The winning chunk is what + # the caller should see (#4243). + won: dict[int, dict] = {} + for system, dist, chunk_index, chunk_text in rows: + score = 1.0 - float(dist) + if system.id not in best or score > best[system.id][0]: + best[system.id] = (score, system) + won[int(system.id)] = { + "index": int(chunk_index), "text": chunk_text or "", + } + ranked = sorted(best.values(), key=lambda pair: pair[0], reverse=True) + kept = [pair for pair in ranked if pair[0] >= threshold][:limit] + record_best_chunk(report, { + int(sys_.id): won[int(sys_.id)] for _s, sys_ in kept if int(sys_.id) in won + }) + return kept + + +async def backfill_system_embeddings() -> None: + """Embed Systems that have no current vectors. Runs at startup beside the + note, rule and milestone backfills; a CHUNKER_VERSION bump re-embeds. + + This is also the pass that makes every existing charter findable at all — + Systems got vectors in #4251, so before it runs there are none.""" + from scribe.models.embedding import SystemEmbedding + from scribe.models.system import System + + try: + async with async_session() as session: + current = select(SystemEmbedding.system_id).where( + SystemEmbedding.chunker_version == CHUNKER_VERSION + ) + stale = (await session.execute( + select(System.id, System.name, System.description) + .where(System.deleted_at.is_(None), System.id.notin_(current)) + )).all() + except Exception: + logger.warning("System embedding backfill: failed to query systems", exc_info=True) + return + + if not stale: + logger.info("System embedding backfill: all systems current at chunker v%d", CHUNKER_VERSION) + return + logger.info("System embedding backfill: embedding %d system(s)", len(stale)) + for system_id, name, description in stale: + await upsert_system_embedding(system_id, name, description) + + async def backfill_milestone_embeddings() -> None: """Embed milestones that have no current vectors. Runs at startup beside the note and rule backfills; a CHUNKER_VERSION bump re-embeds.""" diff --git a/src/scribe/services/systems.py b/src/scribe/services/systems.py index d2f5552..7f2f0bc 100644 --- a/src/scribe/services/systems.py +++ b/src/scribe/services/systems.py @@ -114,6 +114,38 @@ async def seed_standard_systems(user_id: int, project_id: int) -> list[System]: return out +def embed_system(system) -> None: + """Refresh a System's charter vectors, fire-and-forget (#4251). + + The twin of `notes.embed_note`, and here for the same reason (#2056): at + the service, so every door gets it by construction rather than each route + and tool remembering. A charter edited through one door and not another + would stay findable by what it used to say, and nothing would report it. + + Not called on delete. `delete_system` is a SOFT delete and the search joins + through `System`, so a deleted System's vectors are already unreachable — + and leaving them means a restore is findable again immediately instead of + waiting for the next startup backfill. + + Import is lazy so importing this module doesn't pull in the embedding + model; a missing event loop (unit tests, scripts) is ordinary, not an + error; exceptions are swallowed because a System that saved must not fail + on its index refresh. + """ + try: + import asyncio + + from scribe.services.embeddings import upsert_system_embedding + + asyncio.create_task( + upsert_system_embedding(system.id, system.name, system.description) + ) + except RuntimeError: + pass # no running loop — a sync caller, not a failure + except Exception: # noqa: BLE001 - never let indexing break a write + logger.exception("embedding refresh failed for system %s", system.id) + + async def create_system( user_id: int, project_id: int, @@ -144,6 +176,7 @@ async def create_system( session.add(system) await session.commit() await session.refresh(system) + embed_system(system) return system @@ -195,6 +228,7 @@ async def update_system(user_id: int, system_id: int, **fields: object) -> Syste system.updated_at = datetime.now(timezone.utc) await session.commit() await session.refresh(system) + embed_system(system) return system diff --git a/tests/test_mcp_tool_search.py b/tests/test_mcp_tool_search.py index ee167c1..2fff7a7 100644 --- a/tests/test_mcp_tool_search.py +++ b/tests/test_mcp_tool_search.py @@ -1,7 +1,7 @@ """search tool — proves the tool pattern (context + service call + dict shape). Service call is mocked; no DB needed.""" -from unittest.mock import AsyncMock, patch +from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -223,7 +223,8 @@ async def test_the_docstring_names_every_kind_the_tool_accepts(): 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 + for own in ("'rule'", "'milestone'", "'system'", "'all'"): + assert own in doc, f"{own} dispatches somewhere and is never documented" @pytest.mark.asyncio @@ -329,3 +330,80 @@ async def test_milestone_search_is_its_own_shape_and_scopes_to_the_project(): "status": "active", "project_id": 30, "total": 0, "completed": 0, "similarity": 0.81, }] + + +# --- systems: the charter as an answer, not a filter (#4251) ----------------- + + +@pytest.mark.asyncio +async def test_system_search_is_its_own_shape_and_carries_the_matched_passage(): + """A System's charter runs to several hundred words and a result shows its + NAME — so a match on the paragraph that actually decides where a record + belongs would be previewed by two words that cannot. The passage comes + along, marked as the passage (#4243).""" + _user_id_ctx.set(7) + charter = ( + "Opening sentence about the area in general. " + + "x" * 400 + + " Retrieval telemetry and the floors it judges belong here." + ) + fake = MagicMock(id=3, project_id=2, description=charter) + # `name` is MagicMock's own constructor kwarg — passed in, it names the + # mock and leaves `.name` a mock object, which is note #2833's whole point. + fake.name = "Retrieval & recall" + + async def _system_search(uid, q, **kwargs): + kwargs["report"]["best_chunk"] = { + 3: {"index": 2, "text": "Retrieval telemetry and the floors it judges belong here."} + } + return [(0.81, fake)] + + with patch("scribe.mcp.tools.search.semantic_search_systems", _system_search): + out = await search(q="where does retrieval telemetry go?", + content_type="system", project_id=2) + + assert out["total"] == 1 + row = out["results"][0] + assert row["id"] == 3 + assert row["name"] == "Retrieval & recall" + assert row["project_id"] == 2 + assert row["similarity"] == 0.81 + # The passage that won, not the charter's opening — and SAID to be that. + assert row["matched"] == ( + "Retrieval telemetry and the floors it judges belong here." + ) + assert row["matched_is"] == "matched_passage" + assert row["body_length"] == len(charter) + assert "read_full" in row, "a 56-character span of a 500-character charter" + # The charter itself does not ride along: get_system reads it. + assert "description" not in row + + +@pytest.mark.asyncio +async def test_system_search_scopes_to_the_project_it_was_given(): + """Systems are per-project and a charter from another project is not an + answer to "where does this belong here?".""" + _user_id_ctx.set(7) + mock = AsyncMock(return_value=[]) + with patch("scribe.mcp.tools.search.semantic_search_systems", mock): + await search(q="x", content_type="system", project_id=30) + assert mock.call_args.kwargs["project_id"] == 30 + + mock.reset_mock() + with patch("scribe.mcp.tools.search.semantic_search_systems", mock): + await search(q="x", content_type="system") + assert mock.call_args.kwargs["project_id"] is None + + +@pytest.mark.asyncio +async def test_a_system_search_does_not_go_through_the_note_search(): + """Its own search, not a note_type. A charter competing with the whole note + corpus for one top-k is outranked by the records filed under it.""" + _user_id_ctx.set(7) + notes = AsyncMock(return_value=[]) + with ( + patch("scribe.mcp.tools.search.semantic_search_notes", notes), + patch("scribe.mcp.tools.search.semantic_search_systems", AsyncMock(return_value=[])), + ): + await search(q="x", content_type="system") + notes.assert_not_awaited() diff --git a/tests/test_system_charter_search.py b/tests/test_system_charter_search.py new file mode 100644 index 0000000..bc91f23 --- /dev/null +++ b/tests/test_system_charter_search.py @@ -0,0 +1,164 @@ +"""Systems become findable by meaning — the charter as an ANSWER (#4251). + +A System's `description` is a charter: several hundred words saying what +belongs in that area and what does not. It is the answer to "where does this +go?", and there was no semantic path to it — `list_systems` enumerates and +`search(system_id=…)` uses a System as a FILTER over notes, so a System could +narrow a search and could never be the answer to one. + +These pin the three halves: the document shape, the search's scoping and +best-chunk contract, and that a charter edited through any door is re-indexed. +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.services import embeddings as emb +from scribe.services import systems as systems_svc +from tests.helpers import make_mock_session + + +def _system(system_id=3, name="Retrieval & recall", description="the charter"): + s = MagicMock(id=system_id, project_id=2, description=description) + s.name = name # never via the constructor — see note #2833 + return s + + +# --- the document shape ------------------------------------------------------ + + +def test_the_document_is_the_name_and_the_charter(): + assert emb.system_document("Retrieval", "what belongs here") == ( + "Retrieval", "what belongs here" + ) + + +def test_a_system_with_no_charter_yet_degrades_to_its_name(): + """It still embeds, just weakly. That is an argument for writing the + charter, not for padding the document with whatever is to hand.""" + assert emb.system_document("Retrieval", None) == ("Retrieval", None) + assert emb.system_document("Retrieval", " ") == ("Retrieval", None) + assert emb.chunk_document(*emb.system_document("Retrieval", None)) == ["Retrieval"] + + +def test_an_empty_system_produces_no_document_at_all(): + """Callers gate on falsiness to clear vectors rather than embed nothing.""" + assert emb.system_document("", "") == (None, None) + assert emb.chunk_document(*emb.system_document("", "")) == [] + + +# --- the search -------------------------------------------------------------- + + +def _rows_ctx(rows): + session = MagicMock() + result = MagicMock() + result.all.return_value = rows + session.execute = AsyncMock(return_value=result) + ctx = MagicMock() + ctx.__aenter__ = AsyncMock(return_value=session) + ctx.__aexit__ = AsyncMock(return_value=False) + return session, ctx + + +@pytest.mark.asyncio +async def test_the_search_collapses_to_best_chunk_and_reports_which_one_won(): + """A charter is long, and a result shows its NAME — so the paragraph that + actually decides where a record belongs has to come back with it, or the + caller judges from two words that cannot say (#4243). Published from the + start here rather than retrofitted, which is what #4251 asked for.""" + a, b = _system(3), _system(4, name="Data Model") + rows = [ + (a, 0.10, 2, "retrieval telemetry belongs here"), + (b, 0.22, 0, "the persistence layer"), + (a, 0.40, 0, "a weaker paragraph of the same charter"), + ] + _session, ctx = _rows_ctx(rows) + report: dict = {} + with ( + patch.object(emb, "async_session", return_value=ctx), + patch.object(emb, "get_embedding", AsyncMock(return_value=[0.0] * 384)), + patch.object(emb, "can_read_project", AsyncMock(return_value=True)), + ): + out = await emb.semantic_search_systems( + 7, "where does telemetry go?", + project_id=2, threshold=0.0, report=report, + ) + + assert [s.id for _score, s in out] == [3, 4] + assert out[0][0] == 1.0 - 0.10 # the BEST chunk's score + assert report["best_chunk"][3] == { + "index": 2, "text": "retrieval telemetry belongs here", + } + + +@pytest.mark.asyncio +async def test_a_project_the_caller_cannot_read_returns_nothing(): + """Rule 78 — the charter of a project someone was not given is not an + answer to any question they are entitled to ask.""" + _session, ctx = _rows_ctx([]) + with ( + patch.object(emb, "async_session", return_value=ctx), + patch.object(emb, "get_embedding", AsyncMock(return_value=[0.0] * 384)), + patch.object(emb, "can_read_project", AsyncMock(return_value=False)), + ): + assert await emb.semantic_search_systems(7, "q", project_id=99) == [] + + +@pytest.mark.asyncio +async def test_an_empty_query_never_reaches_the_embedder(): + with patch.object(emb, "get_embedding", AsyncMock()) as embed: + assert await emb.semantic_search_systems(7, " ") == [] + embed.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_a_search_that_fails_returns_nothing_rather_than_raising(): + """A recall aid must never break the call it serves.""" + with patch.object(emb, "get_embedding", AsyncMock(side_effect=RuntimeError)): + assert await emb.semantic_search_systems(7, "q") == [] + + +# --- staying current --------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_creating_a_system_indexes_its_charter(): + session = make_mock_session() + with ( + patch.object(systems_svc, "async_session", return_value=session), + patch.object(systems_svc.access, "can_write_project", AsyncMock(return_value=True)), + patch.object(systems_svc, "embed_system") as embed, + ): + await systems_svc.create_system(7, 2, "Retrieval", description="the charter") + embed.assert_called_once() + + +@pytest.mark.asyncio +async def test_editing_a_charter_re_indexes_it(): + """A charter edited and not re-indexed stays findable by what it used to + say — the stale-vector case, and the one that matters most for a record + whose whole job is to say where things belong now.""" + system = _system() + session = make_mock_session() + session.get = AsyncMock(return_value=system) + system.deleted_at = None + with ( + patch.object(systems_svc, "async_session", return_value=session), + patch.object(systems_svc.access, "can_write_project", AsyncMock(return_value=True)), + patch.object(systems_svc, "embed_system") as embed, + ): + await systems_svc.update_system(7, 3, description="a new charter") + embed.assert_called_once_with(system) + + +def test_embedding_a_system_never_breaks_the_write_that_saved_it(): + """A System that saved must not fail on its index refresh. ValueError + rather than RuntimeError deliberately: RuntimeError is also what "no + running loop" raises, which is an ordinary sync caller and has its own + branch — this has to land in the general swallow to prove it exists.""" + with patch( + "scribe.services.embeddings.upsert_system_embedding", + side_effect=ValueError("boom"), + ): + systems_svc.embed_system(_system()) # returns quietly, does not raise -- 2.54.0