diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 8c20748..8081f8d 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -24,6 +24,7 @@ from scribe.services import design_systems as design_systems_svc from scribe.services import knowledge as knowledge_svc from scribe.services import notes as notes_svc from scribe.services import projects as projects_svc +from scribe.services import record_refs as record_refs_svc from scribe.services import shape_ledger as shape_ledger_svc from scribe.services import snippets as snippets_svc from scribe.services import task_claims as task_claims_svc @@ -123,6 +124,34 @@ def _menu_seen_line(note_id: int, kind: str, name: str) -> str: """A pointer to a record this session was already shown, not a copy of it.""" return f"> - #{note_id} [{kind} · seen] {name}" + +# How much of a named record's body sits under its line. The opening, not an +# elided middle: nothing matched here, so there is no passage to keep, and what +# the reader needs is what the record IS — which is where a record says it. +_NAMED_OPENING_CHARS = 240 + + +def _named_opening(body: str | None) -> str: + text = " ".join((body or "").split()) + if len(text) <= _NAMED_OPENING_CHARS: + return text + return text[:_NAMED_OPENING_CHARS].rsplit(" ", 1)[0] + " …" + + +async def _named_records(user_id: int, ids: list[int]) -> list: + """The records behind ids the operator named that this user may open. + + An id that is trashed, someone else's private record or not a record at all + is dropped without a word: the parser cannot tell "4448" the task from + "4448" a quantity it misread, and the lookup is what settles it. + """ + found = [] + for nid in ids: + got = await notes_svc.get_note_for_user(user_id, nid) + if got is not None and got[0].deleted_at is None: + found.append(got[0]) + return found + # Max chars of a Process body to fold into the auto-surface description. _PROC_PREVIEW_CHARS = 200 @@ -1071,6 +1100,13 @@ async def build_autoinject_hint( q = (query or "").strip() if not cfg["enabled"] or not q: return empty + # RECORDS NAMED BY NUMBER (#4796), read from the operator's own words before + # the query is enriched: a number in the assistant's reply is not one the + # operator named. "go ahead with 4448" says which record is meant, and a + # number means nothing to an embedding, so these are looked up rather than + # left to a ranking that can only match the words around them. + named = await _named_records(user_id, record_refs_svc.named_record_ids(q)) + named_ids = [int(n.id) for n in named] # Everything below searches, logs and fills its slots on the ENRICHED # query, so the telemetry row records what was actually asked (#4364). q = _autoinject_query(q, context) @@ -1122,7 +1158,13 @@ async def build_autoinject_hint( # readable — `result_count == 0` with `suppressed_count > 0` is "everything # that matched, this session has already seen", which is a different fact # about the bar from "nothing cleared it" and used to be unreportable here. - fresh = [(s, n) for s, n in hits if int(n.id) not in already] + # A record the operator NAMED is counted the same way: it is shown, but by + # the lookup above and booked under `named_ref`, so this arm did not + # surface it and must not claim it. + fresh = [ + (s, n) for s, n in hits + if int(n.id) not in already and int(n.id) not in named_ids + ] record_retrieval( user_id=user_id, source="auto_inject", query=q, threshold=cfg["threshold"], limit=cfg["top_k"], @@ -1133,36 +1175,89 @@ async def build_autoinject_hint( suppressed=len(hits) - len(fresh), duration_ms=(time.perf_counter() - t0) * 1000.0, ) - if not hits: + if not hits and not named: return empty - # Margin gate: keep only hits close to the strongest one. Computed over ALL - # hits, repeats included — the band measures distance from the top SCORE, - # and letting the ledger move that cutoff would make "you were shown this" - # change what counts as relevant, which is the axis independence the rule - # band keeps for the same reason. - top_score = hits[0][0] - kept = [(s, n) for s, n in hits if s >= top_score - _AUTOINJECT_BAND] - kept = await _reserve_slot_for_reuse( - user_id, q, kept, cfg, project_id=(project_id or None), - already=already, - ) - # AFTER the reuse slot, because that one evicts the menu's weakest hit - # while this one extends: running them the other way round would let a - # reserved lesson be the line reuse throws off, and a slot that another - # slot can silently undo is not a guarantee. - kept, lesson_slot_id = await _reserve_slot_for_lesson( - user_id, q, kept, cfg, project_id=(project_id or None), - already=already, - ) + kept: list = [] + lesson_slot_id = None + if hits: + # Margin gate: keep only hits close to the strongest one. Computed over + # ALL hits, repeats included — the band measures distance from the top + # SCORE, and letting the ledger move that cutoff would make "you were + # shown this" change what counts as relevant, which is the axis + # independence the rule band keeps for the same reason. + top_score = hits[0][0] + kept = [(s, n) for s, n in hits if s >= top_score - _AUTOINJECT_BAND] + # The slots are told about the named records as if already shown, so a + # slot that picks one does not book a surfacing the lookup booked too. + shown = already | set(named_ids) + kept = await _reserve_slot_for_reuse( + user_id, q, kept, cfg, project_id=(project_id or None), + already=shown, + ) + # AFTER the reuse slot, because that one evicts the menu's weakest hit + # while this one extends: running them the other way round would let a + # reserved lesson be the line reuse throws off, and a slot that another + # slot can silently undo is not a guarantee. + kept, lesson_slot_id = await _reserve_slot_for_lesson( + user_id, q, kept, cfg, project_id=(project_id or None), + already=shown, + ) + # A named record is shown once, in its own block, never again as a match. + kept = [(s, n) for s, n in kept if int(n.id) not in named_ids] # A collaborator's note can reach this menu via a shared project, and the # operator never asked for it — so say whose it is. Unattributed, it reads as # something they wrote and settled. owners = await owner_names_for({ - int(n.user_id) for _s, n in kept if n.user_id != user_id + int(n.user_id) for n in [*named, *(n for _s, n in kept)] + if n.user_id != user_id }) + # A superseded record is DEMOTED, not removed (#278) — so one can still reach + # this menu, and when it does the reader has to be told. An agent handed + # stale material with nothing marking it acts on it with full confidence, + # which is worse than never having surfaced it. One query for the whole menu. + stale = await superseded_ids([*named_ids, *(int(n.id) for _s, n in kept)]) + systems = await system_names_for( + {i for i in named_ids if i not in already} + | {int(n.id) for _s, n in kept if int(n.id) not in already} + ) + + lines: list[str] = [] + note_ids: list[int] = [] + # FIRST, because the operator said which record they meant and everything + # after this block is a guess at what else might bear on it. The header + # says these were found by NUMBER: the parser reads "with 4448" as a + # reference, and the reader is the one placed to notice it was a quantity. + if named: + lines.append( + "> Named in your message — the records with those numbers, looked " + "up by id rather than matched. Open any in full with `get_note(id)`; " + "a line marked `seen` is already in your context:" + ) + for note in named: + nid = int(note.id) + note_ids.append(nid) + kind = _record_kind(note) + name = _menu_name(note.title, note.note_type, note.data, note.body) + if nid in already: + line = _menu_seen_line(nid, kind, name) + if nid in stale: + line += " — SUPERSEDED" + lines.append(line) + continue + line = f"> - #{nid} [{_menu_label(kind, systems.get(nid))}] \"{name}\"" + if nid in stale: + line += " — SUPERSEDED, a later record covers this; check that first" + if note.user_id != user_id: + who = owners.get(int(note.user_id)) or "another user" + line += f" — shared by {who}" + lines.append(line) + opening = _named_opening(note.body) + if opening: + lines.append(f"> ↳ {opening}") + # "records", not "notes" — the menu can hold snippets, processes and tasks # too, and the kind marker on each line is only legible if the header doesn't # already claim they're all one thing. @@ -1170,13 +1265,14 @@ async def build_autoinject_hint( # is shown again with a marker rather than withheld, so the header must stop # promising the old contract. It now says what the marker means instead, # once, rather than each repeated line having to explain itself. - lines = [ - "> Possibly relevant from your Scribe records — open any in full with " - "`get_note(id)`, or `get_snippet` / `get_process` / `get_lesson` for " - "those kinds. Each line is a record's name, its kind and System, and " - "the passage that matched; a line marked `seen` is a pointer to one " - "already shown this session, so it is in your context:", - ] + if kept: + lines.append( + "> Possibly relevant from your Scribe records — open any in full with " + "`get_note(id)`, or `get_snippet` / `get_process` / `get_lesson` for " + "those kinds. Each line is a record's name, its kind and System, and " + "the passage that matched; a line marked `seen` is a pointer to one " + "already shown this session, so it is in your context:" + ) # THE REGISTER, SAID ONCE AND ONLY WHEN IT APPLIES (milestone 385 step 5). # # The operator's requirement for this kind was "they don't always have to @@ -1201,21 +1297,15 @@ async def build_autoinject_hint( "what you are doing and use your judgement — a lesson is not a " "rule and binds nothing." ) - # A superseded record is DEMOTED, not removed (#278) — so one can still reach - # this menu, and when it does the reader has to be told. An agent handed - # stale material with nothing marking it acts on it with full confidence, - # 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 {} - systems = await system_names_for({int(n.id) for _s, n in kept if int(n.id) not in already}) - note_ids: list[int] = [] + menu_ids: list[int] = [] for score, note in kept: nid = int(note.id) note_ids.append(nid) + menu_ids.append(nid) kind = _record_kind(note) # The NAME, not the title (#4364): a snippet's or lesson's title is its # embedding shape, trigger and all, and ran past 1,500 characters here. @@ -1270,18 +1360,31 @@ async def build_autoinject_hint( record_surfaced( user_id=user_id, note_ids=[ - i for i in note_ids + i for i in menu_ids if i not in already and i != lesson_slot_id ], source="auto_inject", project_id=project_id, ) + # A lookup, not a ranking, so it writes no retrieval_logs row — there is no + # score or bar to tune — and its surfacings are its own source, which is + # what lets pull-through say whether a named record gets opened. + named_fresh = [i for i in named_ids if i not in already] + if named_fresh: + record_surfaced( + user_id=user_id, note_ids=named_fresh, source="named_ref", + project_id=project_id, + ) # The lessons this menu put in front of the reader, repeats included — for # the soft-link recorder (#4637), which pairs them with the rule arm's # lines in the same response. Relevance, not the session ledger, is what - # makes a co-arrival, so a `seen` lesson counts. - lesson_ids = [int(n.id) for _s, n in kept if _record_kind(n) == LESSON_NOTE_TYPE] + # makes a co-arrival, so a `seen` lesson counts — and so does one the + # operator named, which is relevance by their own say-so. + lesson_ids = [ + int(n.id) for n in [*named, *(n for _s, n in kept)] + if _record_kind(n) == LESSON_NOTE_TYPE + ] return { "context": "\n".join(lines), "note_ids": note_ids, "config": cfg, "lesson_ids": lesson_ids, diff --git a/src/scribe/services/record_refs.py b/src/scribe/services/record_refs.py index fed360e..e2389eb 100644 --- a/src/scribe/services/record_refs.py +++ b/src/scribe/services/record_refs.py @@ -1,4 +1,4 @@ -"""Record references written into prose: refusing guessed ids, resolving placeholders. +"""Record references written into prose: refusing guessed ids, resolving placeholders, reading the ids a message names. THE DEFECT THIS EXISTS FOR (#4016) @@ -129,3 +129,127 @@ def resolve_placeholders(text: str | None, refs: dict[str, str]) -> str | None: if not text: return text return PLACEHOLDER_RE.sub(lambda m: refs[m.group(1)], text) + + +# ── Records an operator names in a message (#4796) ───────────────────────── +# +# "yes go ahead with 4448" says exactly which record is meant, and the prompt +# menu could not use it: a number carries no meaning to an embedding, so the +# menu filled with whatever resembled the words AROUND it. These ids are looked +# up directly instead (plugin_context.build_autoinject_hint). +# +# A guard against a false READ, not a false lookup. A number that is not a +# record id still has to resolve to a record this user can open before anything +# is shown, and the line says it was found by number — but every wrong id is a +# record dropped in front of the reader for no reason, so the parser takes a +# number only where the sentence makes it a reference. + +# How many named records one message can bring in. A real message names one or +# two; past a handful the numbers are a pasted list or a log, not a reference. +NAMED_LIMIT = 5 + +# A number, with its `#` when it has one. The lookbehind keeps out a number +# inside a word, a path or URL (`pulls/198`), a version or date (`1.2`, +# `2026-10-03`), money and entities; the lookahead keeps out the same on the +# other side, a unit glued on (`300ms`, `40%`) and a thousands separator. +_NAMED_RE = re.compile( + r"(?-]*") +_LIST_JOIN_RE = re.compile(r"\s*(?:,|,?\s*(?:and|or|&|\+|/))\s*", re.I) + + +def _prev_word(before: str) -> str: + """The word right before a number, past a space, a quote or a bracket.""" + m = _PREV_WORD_RE.search(before) + return m.group(1).lower() if m else "" + + +def _next_word(after: str) -> str: + m = _NEXT_WORD_RE.match(after) + return m.group(1).lower() if m else "" + + +def named_record_ids(prompt: str | None, limit: int = NAMED_LIMIT) -> list[int]: + """The record ids a message names, in the order it names them. + + `#4448` unless the word before it is another numbering ("PR #198"); a bare + number of three or more digits when it opens the message ("4417 sounds + right"), follows a word that points at something ("go ahead with 4448") or + continues a list one of those started ("4448, 4449 and 4450"), and is not + a quantity ("for 300 seconds"). Fenced code is skipped: a pasted log is + full of numbers and names nothing. + + Read the operator's OWN words, never a query enriched with the assistant's + reply — a number the agent wrote is not one the operator named. + """ + text = _FENCE_RE.sub("\n", prompt or "") + found: list[int] = [] + last_end, last_ok = -1, False + for m in _NAMED_RE.finditer(text): + hashed, digits = bool(m.group(1)), m.group(2) + before, after = text[:m.start()], text[m.end():] + # A list inherits its head's reading, either way: "PR #198 and #199" + # is two PRs, "with 4448, 4449" is two records. + if last_end >= 0 and _LIST_JOIN_RE.fullmatch(text[last_end:m.start()]): + ok = last_ok + elif hashed: + ok = _prev_word(before) not in _OTHER_SPACE + else: + ok = (bool(_OPENS_RE.fullmatch(before)) + or _prev_word(before) in _REF_WORDS) + last_end, last_ok = m.end(), ok + if not ok or digits[0] == "0" or len(digits) < (2 if hashed else 3): + continue + if not hashed and _next_word(after) in _UNITS: + continue + nid = int(digits) + if nid not in found: + found.append(nid) + if len(found) >= limit: + break + return found diff --git a/src/scribe/services/retrieval_registry.py b/src/scribe/services/retrieval_registry.py index 5b2c02c..2b5e914 100644 --- a/src/scribe/services/retrieval_registry.py +++ b/src/scribe/services/retrieval_registry.py @@ -137,6 +137,16 @@ 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"), + # A LOOKUP, not a ranker (#4796): a record the operator named by number + # in the message, fetched by id. No score and no bar, so it writes no + # retrieval_logs row and no score-shaped warning can apply; its rows are + # in `note_usage_events`, where pull-through says whether naming a record + # is followed by opening it. + _p("named_ref", UNBIDDEN, + "a record the operator named by number, looked up directly", + expects_traffic=False, + quiet_because="speaks only when a message names a record by its id; " + "a window in which none did is correctly silent here"), _p("rule_via_lesson", UNBIDDEN, "a rule reached through a lesson confirmed as an instance of it", expects_traffic=False, diff --git a/tests/test_named_records.py b/tests/test_named_records.py new file mode 100644 index 0000000..cade3b5 --- /dev/null +++ b/tests/test_named_records.py @@ -0,0 +1,191 @@ +"""A record the operator names by number is looked up, not left to ranking (#4796). + +"yes go ahead with 4448" says exactly which record is meant, and the prompt +menu could not use it: a number means nothing to an embedding, so the menu +filled with whatever resembled the words around it. The parser cases below are +drawn from real operator prompts, and the half that must NOT be read as a +reference matters as much as the half that must: every misread number is a +stranger's record dropped in front of the reader. +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.services.record_refs import NAMED_LIMIT, named_record_ids +from tests.helpers import fake_note + + +# ── The parser ───────────────────────────────────────────────────────────── + +NAMES = [ + ("yes go ahead with 4448", [4448]), + ("4364 - we had talked about the follow-up case", [4364]), + ("4417 sounds like the right call", [4417]), + ("awesome go for 3898", [3898]), + ("let's continue #4772 then", [4772]), + ("does #12 still hold?", [12]), + ("look at 4448, 4449 and 4450", [4448, 4449, 4450]), + ("with #4448 & #4449", [4448, 4449]), + ("merged (#4794) this morning", [4794]), + ("task 4448 first, then #4449", [4448, 4449]), + ("#4448 again, and #4448", [4448]), # once, in mention order +] + +NOT_NAMES = [ + "merge PR #198 to main", + "PR #198 and #199 are both green", # a list inherits its head + "CI #7855 went red", + "rule 153 says plain merges", + "rule #153 says plain merges", + "milestone 444 is next", + "wait for 300 seconds first", + "about 500 records came back", + "move the budget 3 to 6", + "it ran 7855 times", # no reference word before it + "bump it to 1.2.300", + "on 2026-10-03 it broke", + "see https://forge/x/pulls/198", + "the cut is at 1,000", + "costs $250 a month", + "use 300ms", + "rank #1 was unrelated", # one digit after `#` + "with 42 of them", # two digits, bare + "```\nfor 4448 in range\n```", # fenced code names nothing + "", +] + + +@pytest.mark.parametrize("prompt,want", NAMES, ids=[p for p, _ in NAMES]) +def test_a_named_record_is_read(prompt, want): + assert named_record_ids(prompt) == want + + +@pytest.mark.parametrize("prompt", NOT_NAMES, ids=[p[:30] or "empty" for p in NOT_NAMES]) +def test_a_number_that_is_not_a_reference_is_left(prompt): + assert named_record_ids(prompt) == [] + + +def test_a_long_list_is_capped(): + prompt = "with " + ", ".join(str(1000 + i) for i in range(NAMED_LIMIT + 3)) + assert len(named_record_ids(prompt)) == NAMED_LIMIT + + +# ── The menu ─────────────────────────────────────────────────────────────── + + +async def _hint(prompt, *, hits=(), records=None, seen=(), context=""): + """Run the prompt arm with the search, lookup and recorders stubbed. + + `records` is what the ACL lookup can see, by id; anything else is + inaccessible. + """ + from scribe.services import plugin_context as pc + + records = records or {} + calls: list[int] = [] + + async def _search(*_a, **_kw): + calls.append(1) + return list(hits) if len(calls) == 1 else [] + + async def _get(_uid, nid): + note = records.get(nid) + return (note, "owner") if note is not None else None + + surfaced = MagicMock() + retrieval = MagicMock() + with patch.object(pc, "get_autoinject_config", + AsyncMock(return_value={"enabled": True, "threshold": 0.55, "top_k": 3})), \ + patch.object(pc, "semantic_search_notes", _search), \ + patch.object(pc.notes_svc, "get_note_for_user", AsyncMock(side_effect=_get)), \ + patch.object(pc, "superseded_ids", AsyncMock(return_value=set())), \ + patch.object(pc, "system_names_for", AsyncMock(return_value={})), \ + patch.object(pc, "owner_names_for", AsyncMock(return_value={})), \ + patch.object(pc, "record_retrieval", retrieval), \ + patch.object(pc, "record_surfaced", surfaced): + out = await pc.build_autoinject_hint( + 1, prompt, project_id=2, exclude_ids=list(seen), context=context, + ) + by_source = {c.kwargs["source"]: c.kwargs["note_ids"] for c in surfaced.call_args_list} + return out, by_source, retrieval + + +def _task(nid, title, body=""): + return fake_note(id=nid, title=title, body=body, user_id=1, + is_task=True, status="todo", task_kind="issue") + + +@pytest.mark.asyncio +async def test_a_named_record_arrives_even_when_nothing_matched(): + """The case the whole change is for: a short prompt whose words match + nothing still names its record, and the record arrives.""" + rec = _task(4448, "Filing runs itself after each scan", + "## Symptom\nBooks sat unfiled until someone pressed the button.") + out, surfaced, _ = await _hint("yes go ahead with 4448", records={4448: rec}) + + ctx = out["context"] + assert "Named in your message" in ctx + assert '#4448 [issue (todo)] "Filing runs itself after each scan"' in ctx + assert "↳ ## Symptom Books sat unfiled" in ctx + assert "Possibly relevant" not in ctx, "a menu header over no menu lines" + assert out["note_ids"] == [4448], "the named record must reach the session ledger" + assert surfaced["named_ref"] == [4448] + assert not surfaced.get("auto_inject"), "the ranking arm surfaced nothing here" + + +@pytest.mark.asyncio +async def test_a_named_record_comes_first_and_is_not_repeated_as_a_match(): + named = _task(4448, "the named one") + other = fake_note(id=11, title="something that matched the words", user_id=1) + hits = [(0.80, named), (0.78, other)] + out, surfaced, retrieval = await _hint( + "go ahead with 4448", hits=hits, records={4448: named}, + ) + + ctx = out["context"] + assert ctx.index("Named in your message") < ctx.index("Possibly relevant") + assert ctx.count("#4448") == 1, "the named record was shown twice" + assert out["note_ids"] == [4448, 11] + # Booked once, under the lookup — the ranking arm did not surface it, and + # its log row counts it as suppressed so the row and the usage table agree. + assert surfaced["named_ref"] == [4448] + assert surfaced["auto_inject"] == [11] + row = retrieval.call_args_list[0].kwargs + assert row["source"] == "auto_inject" + assert [int(n.id) for _s, n in row["results"]] == [11] + assert row["suppressed"] == 1 + + +@pytest.mark.asyncio +async def test_a_named_record_already_shown_is_a_pointer(): + rec = _task(4448, "the named one", "a long body that is already in context") + out, surfaced, _ = await _hint("with 4448", records={4448: rec}, seen=[4448]) + + assert "> - #4448 [issue (todo) · seen] the named one" in out["context"] + assert "already in context" not in out["context"] + assert "named_ref" not in surfaced, "a repeat is not a new surfacing" + + +@pytest.mark.asyncio +async def test_an_id_the_reader_cannot_open_is_dropped(): + """Not found, not theirs, or in the trash — all three say nothing, because + the parser cannot tell a record id from a number it misread.""" + trashed = _task(4449, "gone") + trashed.deleted_at = "2026-10-01T00:00:00Z" + out, surfaced, _ = await _hint("with 4448 and 4449", records={4449: trashed}) + + assert out["context"] == "" + assert out["note_ids"] == [] + assert "named_ref" not in surfaced + + +@pytest.mark.asyncio +async def test_a_number_in_the_assistants_reply_is_not_named(): + """The query is enriched with the reply before a short prompt is searched + (#4364); only the operator's own words can name a record.""" + rec = _task(4448, "the named one") + out, _, _ = await _hint( + "sounds good", records={4448: rec}, + context="I could start on #4448 next if you want.", + ) + assert "Named in your message" not in out["context"]