Compare commits
24
Commits
0915c48bb0
..
main
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
cd910e32a0 | ||
|
|
623464323e | ||
|
|
1c4ace5199 | ||
|
|
277f5df515 | ||
|
|
6495236569 | ||
|
|
ab14f783e1 | ||
|
|
1cfbf43ccd | ||
|
|
a165483b92 | ||
|
|
53df6742cb | ||
|
|
e7c1af32a0 | ||
|
|
277aea58e4 | ||
|
|
5e19a1b028 | ||
|
|
be08edcd7e | ||
|
|
7a2aff7bc1 | ||
|
|
0808e8259a | ||
|
|
950c93c5d4 | ||
|
|
21a5831479 | ||
|
|
4a85220aea | ||
|
|
dd1e6e2645 | ||
|
|
14ff41faf5 | ||
|
|
c3ecdf0972 | ||
|
|
1a34363059 | ||
|
|
67df41ae00 | ||
|
|
30d87e461a |
@@ -0,0 +1,62 @@
|
||||
"""add retrieval_logs.best_available_score — the score the bar rejected (#3670)
|
||||
|
||||
Revision ID: 0096
|
||||
Revises: 0095
|
||||
Create Date: 2026-09-08
|
||||
|
||||
`cleared_threshold` was documented as the number to read FIRST — "a surface
|
||||
that clears its bar on nearly every call is either well-tuned or too loose,
|
||||
and p10 says which". It was never a measurement. The search applies the
|
||||
threshold before returning, so every returned result cleared the bar by
|
||||
construction and a call with no results has no `top_score` to compare:
|
||||
the condition is true exactly when `result_count > 0`.
|
||||
|
||||
`zero_result_calls + cleared_threshold == calls` held on all nineteen
|
||||
source/window readings ever taken. It was `calls - zero_result_calls`
|
||||
wearing a name that promised a second opinion, and a reading procedure was
|
||||
built on top of it that asked the reader to compare a number against itself.
|
||||
|
||||
THE MISSING NUMBER, and the reason this is a column rather than a deletion.
|
||||
The question the table exists to answer is "is the bar in the right place",
|
||||
and that question is only answerable from the calls that returned NOTHING:
|
||||
how close did the best rejected candidate come? A bar at 0.72 turning away
|
||||
a stream of 0.71s is set too high by a hair. A bar turning away 0.30s is
|
||||
doing its job. Those two are indistinguishable today — both render as a
|
||||
zero-result call — and no arrangement of the existing columns separates
|
||||
them, because the losing score is discarded inside the search.
|
||||
|
||||
So the searches now rank without the bar and apply it in Python, which
|
||||
costs nothing (the rows were already ordered by distance, and the qualifying
|
||||
set is provably identical — above-threshold rows sort first), and the best
|
||||
score seen becomes observable.
|
||||
|
||||
NULLABLE, AND UNBACKFILLED, for the reason 0095 spells out: a row written
|
||||
before this shipped genuinely does not know what its best rejected candidate
|
||||
scored, and saying so is the honest state. A 0.0 default would read as "the
|
||||
corpus had nothing remotely relevant" — an artifact standing in for a
|
||||
measurement, which is the whole defect this milestone corrects.
|
||||
|
||||
`retrieval_logs` is not restored from backup, so no importer changes.
|
||||
|
||||
Downgrade drops the column. Purely observational — nothing reads it for
|
||||
correctness.
|
||||
"""
|
||||
from alembic import op
|
||||
import sqlalchemy as sa
|
||||
|
||||
|
||||
revision = "0096"
|
||||
down_revision = "0095"
|
||||
branch_labels = None
|
||||
depends_on = None
|
||||
|
||||
|
||||
def upgrade() -> None:
|
||||
op.add_column(
|
||||
"retrieval_logs",
|
||||
sa.Column("best_available_score", sa.Float(), nullable=True),
|
||||
)
|
||||
|
||||
|
||||
def downgrade() -> None:
|
||||
op.drop_column("retrieval_logs", "best_available_score")
|
||||
@@ -1,7 +1,7 @@
|
||||
{
|
||||
"name": "scribe",
|
||||
"description": "Scribe system-of-record for Claude Code: MCP tools over your notes/tasks/projects/rules, a session-start push channel that surfaces your always-on rules + active-project context, process-skills (writing-plans, systematic-debugging, verification, brainstorming, reusing-code), and your saved Scribe Processes auto-surfaced as skills (/scribe:sync). Replaces superpowers + file-memory with one app-backed plugin.",
|
||||
"version": "2026.09.03.0329",
|
||||
"version": "2026.09.09.0408",
|
||||
"author": {
|
||||
"name": "Bryan Van Deusen"
|
||||
},
|
||||
|
||||
@@ -55,6 +55,53 @@ here=$(CDPATH= cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd) || exit 0
|
||||
event=$(cat 2>/dev/null || true)
|
||||
source=$(printf '%s' "$event" | jq -r '.source // empty' 2>/dev/null) || source=""
|
||||
|
||||
# --- The rule ledger outlives the context it describes (#3749) ---
|
||||
#
|
||||
# scribe_prior_art.sh and scribe_tool_rules.sh record every rule id they have
|
||||
# named in <state>/<sid>.rules.ids and hand it back as exclude_rule_ids, so a
|
||||
# rule is named once per session and then goes quiet. That is right while the
|
||||
# session still HOLDS what it was told, and wrong the moment it does not.
|
||||
#
|
||||
# A compaction summarizes the earlier injections away and does not touch the
|
||||
# filesystem, so the rule ends up absent from context AND still excluded —
|
||||
# unreachable for the rest of the session. The banner below tells the model to
|
||||
# re-pull its ALWAYS-ON rules, but a rule an arm surfaced is conditional and is
|
||||
# not in that set, so it has no other way back. The rules most likely to be in
|
||||
# this state are the ones that fire most often, which is to say the ones that
|
||||
# apply most.
|
||||
#
|
||||
# The session id survives a compaction — the etag marker further down is
|
||||
# rewritten on `compact` and keyed by session_id, which is only meaningful if
|
||||
# the id is stable — so the stale ledger is genuinely found again, not orphaned.
|
||||
#
|
||||
# CLEARED ON THE SOURCES THAT DESTROY CONTEXT, AND ONLY THOSE:
|
||||
#
|
||||
# compact CLEAR — summarized away; the file survived.
|
||||
# clear CLEAR — context wiped.
|
||||
# startup nothing to do: a new session id means a new, empty file.
|
||||
# resume KEEP. The context was genuinely restored, so the ledger still
|
||||
# describes what the session holds. Clearing here would re-surface
|
||||
# every rule after a restore that lost nothing — the mirror error.
|
||||
# fork KEEP, and the answer is the same whichever way forks are keyed: a
|
||||
# fork carries the conversation, so if it inherits the id the ledger
|
||||
# is accurate, and if it gets a new one the file is empty anyway.
|
||||
#
|
||||
# ONLY the rules ledger. The same directory holds .ids / .sync.ids /
|
||||
# .derive.ids for the note arms. Whether a surfaced NOTE should return after a
|
||||
# compaction is a different question with a different answer, and leaving those
|
||||
# alone is a decision rather than an oversight.
|
||||
case "$source" in
|
||||
compact|clear)
|
||||
sid=$(printf '%s' "$event" | jq -r '.session_id // empty' 2>/dev/null) || sid=""
|
||||
if [ -n "$sid" ]; then
|
||||
safe_sid=$(printf '%s' "$sid" | tr -c 'A-Za-z0-9._-' '_')
|
||||
# Best-effort, like every other filesystem touch in these hooks: a ledger
|
||||
# that cannot be removed costs a repeated exclusion, never a session.
|
||||
rm -f "${TMPDIR:-/tmp}/scribe-priorart/${safe_sid}.rules.ids" 2>/dev/null || true
|
||||
fi
|
||||
;;
|
||||
esac
|
||||
|
||||
out=""
|
||||
# Append $1 to $out, separated by a horizontal rule when $out already has content.
|
||||
append() { if [ -n "$out" ]; then out="${out}"$'\n\n---\n\n'"$1"; else out="$1"; fi; }
|
||||
|
||||
@@ -21,6 +21,16 @@ for the operator's work, and as your own working memory across sessions.
|
||||
compaction — call `list_always_on_rules()` (and `enter_project()` when a
|
||||
project is in scope) BEFORE acting. When a loaded rule and a default habit
|
||||
disagree, the rule wins; if no rule speaks to it, ask rather than assume.
|
||||
- **What you loaded is not all of the rules.** Only the always-on tier arrives
|
||||
that way; conditional rules are RETRIEVED, and one you were never handed
|
||||
binds exactly as hard. So before a consequential act, `search` for a rule
|
||||
about it (`content_type="rule"`) rather than concluding from an empty
|
||||
loaded set that nothing applies. "I was not told" is not the same as "there
|
||||
is no rule," and only one of those is checkable.
|
||||
This bites hardest on which TOOL to reach for — curling an API that has an
|
||||
MCP client, standing up a local stack, running a suite CI owns. Those feel
|
||||
like mechanics rather than decisions, so they raise no doubt and generate no
|
||||
query; the moment you are most confident is the moment to look.
|
||||
- **Recall before acting** — before you answer anything about the operator's
|
||||
work or start a task, `search` Scribe first; assume a related note, task, or
|
||||
decision already exists. Concretely, reach for recall whenever a request
|
||||
|
||||
@@ -56,10 +56,31 @@ Two constraints on *how* that's achieved:
|
||||
re-deriving it or opening a duplicate. When a project is in scope, pass its
|
||||
`project_id` so results stay scoped.
|
||||
|
||||
2. **Standing rules are binding.** Load them via `list_always_on_rules()` at
|
||||
session start (see "Do this first"); treat every one as binding. Pull a
|
||||
rule's full statement with `get_rule(id)` when it's about to bite. When a
|
||||
project is in scope, `enter_project(id)` also returns its applicable rules.
|
||||
2. **Standing rules are binding — and the ones you were handed are not all of
|
||||
them.** Load the resident set via `list_always_on_rules()` at session start
|
||||
(see "Do this first"); treat every one as binding. Pull a rule's full
|
||||
statement with `get_rule(id)` when it's about to bite. When a project is in
|
||||
scope, `enter_project(id)` also returns its applicable rules.
|
||||
|
||||
Rules come in two tiers. **Always-on** rules are delivered — they arrive
|
||||
whether or not you ask. **Conditional** rules are RETRIEVED, and one binds
|
||||
just as hard for never having been handed to you. So before a consequential
|
||||
act, `search(content_type="rule")` on what you are about to do. An empty
|
||||
loaded set is not evidence that no rule applies; it is only evidence that
|
||||
none was pushed, and those are different claims.
|
||||
|
||||
The tier split exists because delivery does not scale: every resident rule
|
||||
costs tokens in every session forever, so a rulebook that grows past a few
|
||||
dozen either stops growing or stops fitting. Retrieval is what lets the
|
||||
rulebook keep growing — but retrieval only fires if something asks.
|
||||
|
||||
**Ask hardest where you feel most certain.** Rules about which TOOL to reach
|
||||
for — use the forge's MCP client rather than curling its API, don't stand up
|
||||
a local stack, don't run the suite CI owns — govern moves that feel like
|
||||
mechanics rather than decisions. A reflex raises no doubt, so it generates
|
||||
no query, so the rule that would have stopped it is never retrieved. That is
|
||||
the failure this instruction exists to prevent, and confidence is its only
|
||||
warning sign.
|
||||
|
||||
3. **Update over duplicate.** When recording, prefer updating an existing
|
||||
note/rule/task over creating a new one. Search first; revise what's there.
|
||||
|
||||
@@ -45,6 +45,31 @@ from quart import Quart
|
||||
# The accepted cost: an agent that never opens create_note's docstring never
|
||||
# learns the field exists. Guidance lives in the create_note / update_note
|
||||
# docstrings and the using-scribe skill instead.
|
||||
#
|
||||
# Milestone 333 step 3 (2026-09-04) bought the HOW bullet's second clause —
|
||||
# search(content_type="rule") before a consequential act — by TRADING OUT
|
||||
# "Processes are saved procedures (follow verbatim)" and "Deletes are
|
||||
# trash-recoverable". Recorded so the trade is not silently reversed:
|
||||
# - Both were already in test_instruction_surfaces_agree's DISPLACED_TOPICS
|
||||
# and already stated on a delivered surface, so nothing fell off: the
|
||||
# process reflex is in every scribe-proc-* skill listing (each says the
|
||||
# process governs and is followed verbatim), and trash recovery is in the
|
||||
# delete_*/list_trash/restore docstrings, which is where per-tool guidance
|
||||
# belongs by this block's own doctrine.
|
||||
# - What it bought is not per-tool guidance and has nowhere else to live at
|
||||
# session-start altitude. Rules were retrievable only by RESIDENCY: the
|
||||
# always-on preload put them in front of the agent, and nothing told a
|
||||
# session to go looking for one it had not been handed. The tier split is
|
||||
# therefore load-bearing on ANY install (rule 115): a delivered rule costs
|
||||
# tokens in every session forever, so a rulebook that only delivers cannot
|
||||
# grow past what one session can hold, and every rule worth keeping has to
|
||||
# become resident to bind at all. Retrieval is what lets it keep growing —
|
||||
# and retrieval fires only if something asks, which nothing told a session
|
||||
# to do. A tool-choice reflex asks least of all (#3476, #161).
|
||||
# - This states the PULL for conditional rules, exactly as the surrounding
|
||||
# line states it for always-on ones. Rule 119 makes these surfaces the
|
||||
# specification, so the same sentence lands on all three session-start
|
||||
# surfaces, and test_instruction_surfaces_agree pins it.
|
||||
_INSTRUCTIONS = """
|
||||
Scribe is the operator's self-hosted second brain and system of record — and
|
||||
yours: recall from it before acting, record as you go. Keep no parallel copy
|
||||
@@ -63,13 +88,13 @@ Hierarchy: Project -> Milestone -> Task/Note. The map, by purpose:
|
||||
active project_id to stay in scope.
|
||||
- WHERE work happens: Systems. Tag records with system_ids as you write;
|
||||
create_system when the area is unmodelled.
|
||||
- HOW: rules are binding — list_always_on_rules() at session start.
|
||||
- HOW: rules bind. list_always_on_rules() at start; before a consequential
|
||||
act, search(content_type="rule") — the resident set is not all of them.
|
||||
- UI: the project's design system is binding — resolve_design_system /
|
||||
get_design_system_stylesheet before hand-writing a value.
|
||||
- REUSE: search snippets before writing a helper; record what you build with
|
||||
create_snippet; classify shapes against canon (classify_shapes) — a
|
||||
consumer map is rows, never prose. Processes are saved procedures (follow
|
||||
verbatim). Deletes are trash-recoverable.
|
||||
consumer map is rows, never prose.
|
||||
|
||||
A task is a note with status (*_note vs *_task tools).
|
||||
Creates are duplicate-gated: a near-match BLOCKS and returns the existing
|
||||
|
||||
@@ -315,6 +315,61 @@ async def create_rule(
|
||||
) -> dict:
|
||||
"""Create a new rule in a rulebook (a SHARED rule — keep it general).
|
||||
|
||||
PROPOSE RULES READILY, AND WRITE ONE WHEN THE OPERATOR SAYS YES. Noticing
|
||||
that something has hardened into a standing instruction is valuable work,
|
||||
and a session that notices it and says nothing has thrown the observation
|
||||
away. So raise it whenever you see one. The single step that belongs
|
||||
between noticing and writing is the operator's yes: a rule binds every
|
||||
future session, and they are the person it binds.
|
||||
|
||||
Their yes is also the only moment the rule is reliably IN FRONT of them.
|
||||
After the write it may not be again for months — a conditional rule is not
|
||||
read aloud at session start, and a project-scoped one does not appear in
|
||||
an unfiltered list_rules() at all. So the proposal is the review.
|
||||
|
||||
When the operator asks for a rule in so many words, that IS the yes —
|
||||
write it and move on. The loop below is for the rule you thought of.
|
||||
|
||||
A PROPOSAL CARRIES FOUR THINGS, and the fourth is the one that decides it:
|
||||
|
||||
1. WHAT it would require — the statement, in the words it would carry,
|
||||
not a gloss of them. The operator is agreeing to text.
|
||||
2. INTENT — what it changes about how work gets done, and what goes
|
||||
wrong today without it. "Be careful about X" is not an intent; the
|
||||
behaviour that would differ tomorrow is.
|
||||
3. WHY NOW — the incident, observation or decision behind it. Pass that
|
||||
record as arose_from_id, and say it in the conversation too: the
|
||||
field is for the reader six months out, the sentence is for the
|
||||
person deciding.
|
||||
4. HOW IT WOULD BE ENFORCED — a test, a CI check, a hook, a schema
|
||||
constraint, a duplicate gate, a review step... or nothing, in which
|
||||
case say so plainly: "nothing — this is prose a session has to
|
||||
remember." Answer this one honestly and it will sometimes dissolve
|
||||
the rule, which is the point rather than a side effect. What a test
|
||||
can assert should BE that test; a rule is what remains when nothing
|
||||
mechanical can hold the thing. A rulebook grows by default and
|
||||
shrinks only on purpose, so a question that prevents a rule is worth
|
||||
more than any question that improves one's wording.
|
||||
|
||||
THEN CLOSE WITH A QUESTION THEY CAN ANSWER IN ONE WORD. Offer three
|
||||
answers, and make the middle one the easy one:
|
||||
|
||||
* "Approve it AS WRITTEN" — you create it with the statement exactly as
|
||||
shown. This is what makes element 1 load-bearing: they approved TEXT,
|
||||
so that text is what gets stored, verbatim.
|
||||
* "LET'S TALK ABOUT IT" — the wording, the scope, the tier, whether it
|
||||
wants to be a rule at all. Most good rules arrive this way, so treat
|
||||
this answer as the expected one rather than a setback.
|
||||
* "NO" — let it go. If the observation is still worth keeping, it is a
|
||||
note (create_note): recorded, findable, and binding on nobody.
|
||||
|
||||
Where the interface offers structured choices, ask it that way — a
|
||||
question with named options is answered in a click, while the same
|
||||
question inside a paragraph is answered by scrolling past. Where it does
|
||||
not, write the three options out as three options. Either way ask once
|
||||
and let the answer stand; re-raising a declined proposal argues a rule
|
||||
into existence, which is the thing this whole loop exists to prevent.
|
||||
|
||||
A rulebook rule is shared by every project that gets the rulebook: an
|
||||
always_on rulebook binds ALL your projects; a subscribed rulebook binds the
|
||||
projects that opt in. So a rulebook rule must read as a general standard —
|
||||
@@ -433,6 +488,16 @@ async def create_project_rule(
|
||||
the rule is returned in get_project's applicable_rules (under
|
||||
project_rules) and in list_rules(project_id=...).
|
||||
|
||||
PROPOSE, THEN WRITE ON A YES — create_rule's opening carries the whole
|
||||
loop: the four things a proposal states (what it would require, its
|
||||
intent, why now, and how it would be enforced) and the one-word question
|
||||
that closes it (approve as written / talk about it / no). All of it
|
||||
applies here unchanged. Reach for that loop MORE readily on this surface,
|
||||
not less: a project rule stays out of an unfiltered list_rules(), and a
|
||||
conditional one stays out of session start too, so the operator's yes is
|
||||
the one moment this rule is certain to have been seen by the person it
|
||||
binds.
|
||||
|
||||
Check first whether a rule is the right shape at all — create_rule's
|
||||
opening asks that question and it applies identically here. A visual
|
||||
standard is a design system; a procedure is a process (create_process);
|
||||
|
||||
@@ -112,6 +112,7 @@ async def search(
|
||||
return await _search_rules(uid, q, limit)
|
||||
is_task = {"note": False, "task": True}.get(content_type) # None => any
|
||||
t0 = time.perf_counter()
|
||||
report: dict = {}
|
||||
raw = await semantic_search_notes(
|
||||
uid, q, limit=limit, is_task=is_task,
|
||||
project_id=project_id or None,
|
||||
@@ -119,12 +120,15 @@ async def search(
|
||||
# An explicit search reaches everything the operator may read, including
|
||||
# records shared with them one-to-one.
|
||||
scope="read",
|
||||
report=report,
|
||||
)
|
||||
record_retrieval(
|
||||
user_id=uid, source="mcp_search", query=q,
|
||||
threshold=DEFAULT_SIMILARITY_THRESHOLD, limit=limit,
|
||||
project_id=project_id or None, is_task=is_task, results=raw,
|
||||
duration_ms=(time.perf_counter() - t0) * 1000.0,
|
||||
best_available=report.get("best_available_score"),
|
||||
searched=bool(report.get("searched", True)),
|
||||
)
|
||||
owners = await owner_names_for(
|
||||
{int(note.user_id) for _s, note in raw if note.user_id != uid}
|
||||
@@ -162,18 +166,51 @@ async def retrieval_telemetry(days: int = 30) -> dict:
|
||||
|
||||
`sources` — per retrieval surface (`auto_inject`, `write_path`,
|
||||
`mcp_search`, …), from `retrieval_logs`: `calls`, `zero_result_calls`,
|
||||
`cleared_threshold` (how often the best hit beat the threshold in force for
|
||||
that call), the `top_score` spread (p10/p50/p90/min/max), `avg_result_count`
|
||||
and `p90_duration_ms`. THE number to read first is `cleared_threshold`
|
||||
against `calls`, with the spread beside it: a surface that clears its bar
|
||||
on nearly every call is either well-tuned or too loose, and p10 says which.
|
||||
`near_misses`, the `top_score` spread (p10/p50/p90/min/max),
|
||||
`avg_result_count` and `p90_duration_ms`.
|
||||
|
||||
READ `cleared_threshold` AND `zero_result_calls` TOGETHER, and check
|
||||
`suppression` before concluding anything from either. A zero-result call is
|
||||
two different events wearing one number: the ranker found nothing above the
|
||||
bar, or it found only what this session had already been shown. Just the
|
||||
first is evidence the bar is too high. `suppression` splits them where the
|
||||
surface can tell — `zero_because_already_shown` comes off
|
||||
THE NUMBER TO READ FIRST IS `near_misses.p90`, AGAINST THE THRESHOLD IN
|
||||
FORCE FOR THAT SURFACE. It is measured on the calls the BAR turned away —
|
||||
zero-result calls, minus the ones whose zero was a repeat the reader had
|
||||
already been shown — using the best score the ranker reached before the bar
|
||||
rejected it. So it is the one figure here that says something the bar
|
||||
cannot make true by construction, and `max` is always below the threshold:
|
||||
an above-bar candidate nobody excluded would have been returned. A bar at 0.72 turning away a stream of 0.71s is set too
|
||||
high by a hair and the surface is losing hits it should have had. The same
|
||||
bar turning away 0.30s is working, and the corpus simply had nothing. Both
|
||||
render as a zero-result call, and nothing else in this readout tells them
|
||||
apart.
|
||||
|
||||
`near_misses` is `null` when no declining call in the window measured it —
|
||||
rows written before #3670 shipped cannot know. That is "not measured", not
|
||||
"nothing came close"; a 0.0 there would be a claim about the corpus
|
||||
invented out of a caller's silence.
|
||||
|
||||
A NULL HERE NOW MEANS ONE THING, which it did not at first. A semantic
|
||||
search returns nothing three ways WITHOUT having run — an empty query, an
|
||||
unavailable embedder, and a failed database query — and each used to write
|
||||
a row indistinguishable from a ranker that declined (#3765). Those calls no
|
||||
longer write a row at all, on the same reasoning that already keeps a blank
|
||||
command out of the log: a row there reports a call that never happened and
|
||||
drags the clear rate down with phantom declines. So a null is "searched,
|
||||
and nothing came close", and a broken search shows up as a WARNING in the
|
||||
application log rather than as a quiet zero in here.
|
||||
|
||||
THERE IS NO `cleared_threshold` ANY MORE, and if you remember one, that
|
||||
memory is of a tautology (#3670). The search applies the bar before
|
||||
returning, so every returned result cleared it by construction and a call
|
||||
with no results has no score to compare: the field was true exactly when
|
||||
`result_count > 0`, i.e. it was `calls - zero_result_calls` under a name
|
||||
that promised a second opinion. `zero_result_calls + cleared_threshold ==
|
||||
calls` held on all nineteen readings ever taken. The reading procedure
|
||||
built on it — "clears its bar on nearly every call" — asked you to compare
|
||||
a number with itself.
|
||||
|
||||
CHECK `suppression` BEFORE CONCLUDING ANYTHING FROM `zero_result_calls`. A
|
||||
zero-result call is two different events wearing one number: the ranker
|
||||
found nothing above the bar, or it found only what this session had already
|
||||
been shown. Just the first is evidence about the bar. `suppression` splits
|
||||
them where the surface can tell — `zero_because_already_shown` comes off
|
||||
`zero_result_calls` to leave the true ranker declines.
|
||||
|
||||
`suppression` is `null` when NO row in the window reported it, and that is
|
||||
@@ -237,10 +274,37 @@ It is an UPPER BOUND per surface: a pull records the door it came
|
||||
those same rules over time: a resident set surfaced thousands of times and
|
||||
opened never is the dead-weight signal, one tier up.
|
||||
|
||||
Read it against `sources["write_path_rule"]`. That surface has never once
|
||||
declined to fire, and until this block existed there was no way to tell a
|
||||
well-tuned arm from a bar it cannot fail to clear (#3311). `pull_through`
|
||||
is the number that tells them apart.
|
||||
Read it against `sources["write_path_rule"]`. That arm was once believed
|
||||
never to decline — the reading that scoped #3311 — but it was the arm's
|
||||
`retrieval_logs` row being written only on calls that FOUND something, so
|
||||
the zeros were missing rather than absent (#3497). Measured since, it
|
||||
declines the large majority of its calls like any other surface.
|
||||
|
||||
EVERY COUNTER BLOCK CARRIES ITS OWN COVERAGE — `complete_from` and
|
||||
`covers_window`. `complete_from` is when the number became trustworthy:
|
||||
for one source, its first recorded row; for a section that sums several,
|
||||
the LATEST of theirs, because a total is complete only once every
|
||||
contributor was being written. `covers_window: false` means the window
|
||||
reaches back further than the recording does, so the count is a fraction
|
||||
of the period it appears to describe.
|
||||
|
||||
READ IT BEFORE COMPARING TWO NUMBERS, and especially before comparing
|
||||
across a deploy. A counter added last week, read over a 30-day window,
|
||||
reports a real count against an imagined denominator — and the result is
|
||||
a plausible fraction rather than an obvious zero, which is what makes it
|
||||
dangerous. That reading cost milestone #379 five steps aimed at a defect
|
||||
that did not exist.
|
||||
|
||||
`covers_window` is null, never false, when nothing was ever recorded:
|
||||
"no measurement" is not "partial measurement", the same distinction
|
||||
`suppression`'s null carries a few paragraphs up.
|
||||
|
||||
A SOURCE SHOWING `calls: 0` WAS RECORDING AND MADE NO CALLS. `sources`
|
||||
lists every source the table has ever held, not only those active in the
|
||||
window, so a surface that stopped firing stays visible rather than
|
||||
disappearing — being absent is reserved for a source that has never
|
||||
recorded at all. Its score fields are null, not zero: the calls are a
|
||||
real observation, the distribution is not one.
|
||||
|
||||
`rule_usage_failed: true` means that read failed while the rest of the
|
||||
readout stood. The counts are still present so a caller can render, but
|
||||
|
||||
@@ -54,6 +54,14 @@ class RetrievalLog(Base):
|
||||
suppressed_count: Mapped[int | None] = mapped_column(Integer, nullable=True)
|
||||
top_score: Mapped[float | None] = mapped_column(Float, nullable=True)
|
||||
min_score: Mapped[float | None] = mapped_column(Float, nullable=True)
|
||||
# The best score the ranker COULD have offered, before the threshold — as
|
||||
# against `top_score`, which is the best it DID offer. They are equal on
|
||||
# any call that returned something, and only this one exists on a call
|
||||
# that returned nothing, which is the only place a bar can be judged from
|
||||
# (#3670). Null means the caller did not measure it, never "nothing was
|
||||
# close": a 0.0 there would read as a corpus with no relevant records at
|
||||
# all, which is an artifact standing in for a measurement.
|
||||
best_available_score: Mapped[float | None] = mapped_column(Float, nullable=True)
|
||||
# [{"id": int, "score": float, "rank": int}, ...], highest-first.
|
||||
result_ids: Mapped[list] = mapped_column(JSONB, nullable=False, default=list)
|
||||
duration_ms: Mapped[float | None] = mapped_column(Float, nullable=True)
|
||||
|
||||
@@ -44,17 +44,21 @@ async def search_route():
|
||||
project_id = request.args.get("project_id", type=int)
|
||||
|
||||
t0 = time.perf_counter()
|
||||
report: dict = {}
|
||||
results = await semantic_search_notes(
|
||||
uid, q, limit=limit, is_task=is_task, 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",
|
||||
report=report,
|
||||
)
|
||||
record_retrieval(
|
||||
user_id=uid, source="rest_search", query=q,
|
||||
threshold=_REST_SEARCH_THRESHOLD, limit=limit,
|
||||
project_id=project_id, is_task=is_task, results=results,
|
||||
duration_ms=(time.perf_counter() - t0) * 1000.0,
|
||||
best_available=report.get("best_available_score"),
|
||||
searched=bool(report.get("searched", True)),
|
||||
)
|
||||
owners = await owner_names_for(
|
||||
{int(note.user_id) for _s, note in results if note.user_id != uid}
|
||||
|
||||
@@ -448,6 +448,23 @@ async def upsert_note_embedding(
|
||||
logger.warning("Failed to persist embedding for note %d", note_id, exc_info=True)
|
||||
|
||||
|
||||
# Both searches rank WITHOUT the threshold and apply it in Python, so the best
|
||||
# rejected score stays observable (#3670). The qualifying set is provably
|
||||
# unchanged: rows arrive ordered by distance ascending, so every above-bar row
|
||||
# sorts ahead of every below-bar one, and an over-fetch that used to return N
|
||||
# above-bar rows returns the same N plus some losers. What changes is only that
|
||||
# the losers are now visible instead of discarded inside the query.
|
||||
#
|
||||
# That visibility is the entire point. A bar can only be judged from the calls
|
||||
# it TURNED AWAY — a 0.72 bar rejecting a stream of 0.71s is set too high by a
|
||||
# hair, one rejecting 0.30s is working — and those two are indistinguishable
|
||||
# from any arrangement of the columns that survive the filter.
|
||||
#
|
||||
# `report` is how the score gets out without changing what a search RETURNS.
|
||||
# Eight of the eleven call sites want hits and nothing else; the three that
|
||||
# write telemetry pass a dict and read `best_available_score` back out of it.
|
||||
|
||||
|
||||
async def semantic_search_notes(
|
||||
user_id: int,
|
||||
query: str,
|
||||
@@ -462,12 +479,27 @@ async def semantic_search_notes(
|
||||
scope: str = "own",
|
||||
demote_superseded: bool = True,
|
||||
system_id: int | None = None,
|
||||
report: dict | None = None,
|
||||
) -> list[tuple[float, Note]]:
|
||||
"""Return up to *limit* (score, note) pairs most relevant to *query*.
|
||||
|
||||
Scores are cosine similarities in [-1, 1]; only notes at or above
|
||||
*threshold* are returned, sorted highest-first.
|
||||
|
||||
Pass `report` (an empty dict) to learn what the threshold turned away:
|
||||
the function sets `report["best_available_score"]` to the highest score
|
||||
anything reached, or None when the corpus offered nothing at all. It is
|
||||
the only figure that survives a call returning nothing, and therefore the
|
||||
only one a bar can be judged from (#3670).
|
||||
|
||||
It also sets `report["searched"]`: False before anything can return, True
|
||||
only where a real result set exists. So an empty query, an unavailable
|
||||
embedder and a failed database query all leave it FALSE, and a caller can
|
||||
tell a search that found nothing from one that never ran. A caller logging
|
||||
telemetry must check it — recording a failed search as a zero-result call
|
||||
reports a decline the ranker never made (#3765). ABSENT means no search
|
||||
touched the dict at all, which is a stand-in in a test, not a real call.
|
||||
|
||||
`note_type` narrows to a record kind, or several (e.g. "snippet", or
|
||||
("snippet", "note")), for callers that want prior art rather than everything
|
||||
embedded.
|
||||
@@ -502,6 +534,13 @@ async def semantic_search_notes(
|
||||
|
||||
Returns an empty list if the embedder is unavailable or on any error.
|
||||
"""
|
||||
# Stamped FALSE before anything can return, flipped True only where a real
|
||||
# result set exists (#3765). Every early return below leaves it false, so a
|
||||
# caller can tell a search that found nothing from one that never ran. It
|
||||
# has to be the first thing done to `report`: a return added above this
|
||||
# line would leave the key ABSENT, which reads as "no caller asked".
|
||||
if report is not None:
|
||||
report["searched"] = False
|
||||
if not query or not query.strip():
|
||||
return []
|
||||
try:
|
||||
@@ -513,7 +552,6 @@ async def semantic_search_notes(
|
||||
# Distance ceiling equivalent to the similarity floor. Clamp to the valid
|
||||
# cosine-distance range [0, 2] so a threshold of, say, -1 doesn't produce a
|
||||
# nonsensical ceiling.
|
||||
max_distance = min(2.0, max(0.0, 1.0 - threshold))
|
||||
distance = NoteEmbedding.embedding.cosine_distance(query_vec)
|
||||
|
||||
try:
|
||||
@@ -588,11 +626,10 @@ async def semantic_search_notes(
|
||||
fetch = limit * _CHUNK_OVERFETCH * (
|
||||
_SUPERSESSION_OVERFETCH if demote_superseded else 1
|
||||
)
|
||||
stmt = (
|
||||
stmt.where(distance <= max_distance)
|
||||
.order_by(distance.asc())
|
||||
.limit(fetch)
|
||||
)
|
||||
# NO threshold predicate — see the note above this function. The
|
||||
# bar is applied after the collapse, where the rejected scores can
|
||||
# still be seen.
|
||||
stmt = stmt.order_by(distance.asc()).limit(fetch)
|
||||
rows = list((await session.execute(stmt)).all())
|
||||
except Exception:
|
||||
logger.warning("Failed to query note embeddings", exc_info=True)
|
||||
@@ -611,6 +648,18 @@ async def semantic_search_notes(
|
||||
continue
|
||||
seen.add(int(note.id))
|
||||
scored.append((1.0 - float(dist), note))
|
||||
# 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:
|
||||
# `searched` is what stops a null score meaning four things (#3765).
|
||||
# Every early return above — empty query, embedder down, and the broad
|
||||
# `except` around the query itself — leaves this key ABSENT, so a
|
||||
# caller can tell "I looked and there was nothing" from "I never
|
||||
# looked" and from "the query failed". Set here, at the one point past
|
||||
# which a real result set exists.
|
||||
report["searched"] = True
|
||||
report["best_available_score"] = scored[0][0] if scored 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)
|
||||
@@ -764,9 +813,24 @@ async def semantic_search_rules(
|
||||
limit: int = 5,
|
||||
threshold: float = _SIMILARITY_THRESHOLD,
|
||||
tier: str | None = None,
|
||||
report: dict | None = None,
|
||||
) -> list[tuple[float, "Rule"]]:
|
||||
"""Return up to *limit* (score, rule) pairs most relevant to *query*.
|
||||
|
||||
Pass `report` (an empty dict) to learn what the threshold turned away:
|
||||
the function sets `report["best_available_score"]` to the highest score
|
||||
anything reached, or None when the corpus offered nothing at all. It is
|
||||
the only figure that survives a call returning nothing, and therefore the
|
||||
only one a bar can be judged from (#3670).
|
||||
|
||||
It also sets `report["searched"]`: False before anything can return, True
|
||||
only where a real result set exists. So an empty query, an unavailable
|
||||
embedder and a failed database query all leave it FALSE, and a caller can
|
||||
tell a search that found nothing from one that never ran. A caller logging
|
||||
telemetry must check it — recording a failed search as a zero-result call
|
||||
reports a decline the ranker never made (#3765). ABSENT means no search
|
||||
touched the dict at all, which is a stand-in in a test, not a real call.
|
||||
|
||||
Scoped by OWNERSHIP — a rule is the caller's if they own its rulebook or
|
||||
its project. Deliberately not filtered to what currently BINDS a given
|
||||
project: this answers "is there a rule about this", which a person asking
|
||||
@@ -774,10 +838,17 @@ async def semantic_search_rules(
|
||||
is the surfacing question, and it has its own machinery
|
||||
(get_applicable_rules) rather than a second, subtly different copy here.
|
||||
|
||||
`tier` narrows to one tier. The write-path hint passes "conditional",
|
||||
because an always-on rule is ALREADY in the session — surfacing it again as
|
||||
a suggestion is pure noise, and noise on a hint that fires on every write
|
||||
is how a hint gets ignored.
|
||||
`tier` narrows to one tier, and NONE is the ordinary case. The write-path
|
||||
and pre-tool hints deliberately pass nothing: an always-on rule is already
|
||||
in the session, but being in a list from turn zero is not the same as being
|
||||
in front of the reader when the action it governs is taken, and filtering
|
||||
on tier made a whole class of rules permanently ineligible for the one
|
||||
mechanism that surfaces a rule AT the moment. Relevance is the threshold's
|
||||
job; see the block above RULEHINT_LIMIT in services/plugin_context.py for
|
||||
the argument and for what the resulting scores are being read against.
|
||||
|
||||
Pass a tier when a caller genuinely wants one class — a listing, an audit,
|
||||
a UI that renders the tiers apart. Not to approximate relevance.
|
||||
|
||||
Collapses to best-chunk-per-rule like the note search, so a long rule split
|
||||
across chunks competes once rather than crowding the results with itself.
|
||||
@@ -787,6 +858,9 @@ async def semantic_search_rules(
|
||||
from scribe.models.project import Project
|
||||
from scribe.models.rulebook import Rule, Rulebook, RulebookTopic
|
||||
|
||||
# See the sibling search: stamped before anything can return (#3765).
|
||||
if report is not None:
|
||||
report["searched"] = False
|
||||
if not query or not query.strip():
|
||||
return []
|
||||
try:
|
||||
@@ -795,7 +869,6 @@ async def semantic_search_rules(
|
||||
logger.debug("Rule search skipped — embedder unavailable")
|
||||
return []
|
||||
|
||||
max_distance = min(2.0, max(0.0, 1.0 - threshold))
|
||||
distance = RuleEmbedding.embedding.cosine_distance(query_vec)
|
||||
|
||||
try:
|
||||
@@ -809,7 +882,8 @@ async def semantic_search_rules(
|
||||
.outerjoin(Project, Rule.project_id == Project.id)
|
||||
.where(
|
||||
Rule.deleted_at.is_(None),
|
||||
distance <= max_distance,
|
||||
# No threshold predicate — see the note above
|
||||
# semantic_search_notes. Applied below, after the collapse.
|
||||
# topic_id XOR project_id, so exactly one arm can match.
|
||||
or_(
|
||||
Rulebook.owner_user_id == user_id,
|
||||
@@ -832,7 +906,11 @@ async def semantic_search_rules(
|
||||
if rule.id not in best or score > best[rule.id][0]:
|
||||
best[rule.id] = (score, rule)
|
||||
ranked = sorted(best.values(), key=lambda pair: pair[0], reverse=True)
|
||||
return ranked[:limit]
|
||||
if report is not None:
|
||||
# See the sibling search: absent means the search never ran (#3765).
|
||||
report["searched"] = True
|
||||
report["best_available_score"] = ranked[0][0] if ranked else None
|
||||
return [pair for pair in ranked if pair[0] >= threshold][:limit]
|
||||
|
||||
|
||||
async def backfill_rule_embeddings() -> None:
|
||||
|
||||
@@ -94,13 +94,17 @@ WRITEPATH_DEFAULT_THRESHOLD = 0.68
|
||||
# THE STRUCTURAL ARGUMENT, which is the only kind admissible here (rule 115).
|
||||
# Two facts hold on any install, including one with six rules and no telemetry:
|
||||
#
|
||||
# 1. The eligible corpus is TINY. The arm searches `tier="conditional"`
|
||||
# rules only — a handful to a few dozen documents against thousands of
|
||||
# notes. A top-k over forty candidates always returns something, so
|
||||
# "the best match cleared the bar" stops meaning "a good match exists"
|
||||
# and starts meaning "forty things were ranked". A bar calibrated for
|
||||
# best-of-thousands is cleared by best-of-forty as arithmetic, not
|
||||
# relevance.
|
||||
# 1. The eligible corpus is SMALL — every rule an install owns, still only
|
||||
# a few dozen documents against thousands of notes. A top-k over a small
|
||||
# pool always returns something, so "the best match cleared the bar"
|
||||
# drifts from "a good match exists" toward "N things were ranked". A bar
|
||||
# calibrated for best-of-thousands is cleared by best-of-forty as
|
||||
# arithmetic rather than relevance.
|
||||
# This argument WEAKENED when the arms stopped filtering to one tier
|
||||
# (see the note on that below): a larger pool makes clearing the bar
|
||||
# mean more, not less. The threshold was deliberately left where it was
|
||||
# anyway — moving two variables at once would make the resulting
|
||||
# distribution unreadable, and this one errs toward silence on purpose.
|
||||
# 2. Rules are short imperative technical English — a far more HOMOGENEOUS
|
||||
# corpus than note prose. #2223 measured the floor for code against prose
|
||||
# at 0.55-0.63 and set 0.68 above it. A more homogeneous corpus has a
|
||||
@@ -128,9 +132,14 @@ RULEHINT_DEFAULT_THRESHOLD = 0.72
|
||||
# ONE rule per write, not two — and this is deliberately NOT a knob.
|
||||
#
|
||||
# With a corpus this small, top-k does as much damage as the threshold: k=2
|
||||
# over forty candidates means the second line is almost always the second-best
|
||||
# noise, arriving with the same confident framing as the first. Halving k
|
||||
# halves that regardless of where the bar sits.
|
||||
# over a few dozen candidates means the second line is almost always the
|
||||
# second-best noise, arriving with the same confident framing as the first.
|
||||
# Halving k halves that regardless of where the bar sits.
|
||||
#
|
||||
# It also BOUNDS the blast radius of widening the pool (below): with k=1 a
|
||||
# wider corpus can change WHICH rule surfaces and how often one does, but it
|
||||
# can never make a single hint longer. The loudness of one hint and the
|
||||
# eligibility of a rule are separate controls, and only one of them moved.
|
||||
#
|
||||
# It stays a constant because it is a decision about how LOUD one hint may be,
|
||||
# not a per-install tuning question. The hint already carries prior art, shape
|
||||
@@ -140,6 +149,45 @@ RULEHINT_DEFAULT_THRESHOLD = 0.72
|
||||
# adds a way to misconfigure the surface (rule 25 cuts both ways).
|
||||
RULEHINT_LIMIT = 1
|
||||
|
||||
# WHY THE ARMS NO LONGER FILTER TO ONE TIER (#3702).
|
||||
#
|
||||
# Both arms used to pass `tier="conditional"`, on the reasoning that an
|
||||
# always-on rule is already in the session, so surfacing it again is pure
|
||||
# noise. That reasoning conflates two different things:
|
||||
#
|
||||
# PRESENT IN CONTEXT — the rule was delivered at session start.
|
||||
# SALIENT AT THE MOMENT — the rule is in front of the reader when the
|
||||
# action it governs is about to be taken.
|
||||
#
|
||||
# A rule handed over in a list at turn zero is present while a session writes
|
||||
# a config value three hundred turns later. It is not surfaced. So the filter
|
||||
# did not merely skip a redundant hint — it made a whole class of rules
|
||||
# permanently ineligible for the only mechanism that puts a rule in front of
|
||||
# an agent AT the moment, and the more important a rule is, the more likely
|
||||
# it was in that class.
|
||||
#
|
||||
# The deeper defect is that the filter was doing the THRESHOLD's job. Whether
|
||||
# a rule belongs in this hint is a relevance question, and a similarity bar is
|
||||
# the control for relevance. A categorical exclusion standing in for a
|
||||
# relevance judgment cannot be tuned, cannot be measured, and cannot be wrong
|
||||
# in a way anybody notices.
|
||||
#
|
||||
# THIS IS A MEASURED CHANGE, NOT A SETTLED ONE. The old comment's fear is
|
||||
# real — a hint that fires on every write and says obvious things teaches the
|
||||
# reader to skip the block, and the surface is then lost along with its true
|
||||
# positives. That fear had simply never been checked. `retrieval_logs` already
|
||||
# records top_score, result_count and the query for every call, so the
|
||||
# evidence now arrives on its own:
|
||||
#
|
||||
# - rules clear the bar often and at high scores -> the fear was justified,
|
||||
# the filter was a crude proxy for a bar set too low, and the WORK IS THE
|
||||
# BAR. Any reinstated filter should then carry a measured reason.
|
||||
# - rules clear rarely, in a thin band near the bar -> the filter was never
|
||||
# the right instrument and relevance was always sufficient.
|
||||
#
|
||||
# Only the eligibility moved. The bar and k=1 were both left exactly where
|
||||
# they were, so the resulting distribution has one cause.
|
||||
|
||||
# How much of a command reaches the embedding (#3476). A shell call is not a
|
||||
# file: most are short, and the ones that are not are usually a heredoc or a
|
||||
# pasted script whose bulk says nothing about which rule applies. The VERB AND
|
||||
@@ -415,6 +463,7 @@ async def _reserve_slot_for_reuse(
|
||||
|
||||
top_k = cfg["top_k"]
|
||||
_t0 = time.perf_counter()
|
||||
_rep: dict = {}
|
||||
reuse = await semantic_search_notes(
|
||||
user_id, query,
|
||||
limit=1,
|
||||
@@ -423,6 +472,7 @@ async def _reserve_slot_for_reuse(
|
||||
exclude_ids=exclude_ids | {int(n.id) for _s, n in kept},
|
||||
note_type=_REUSE_KINDS,
|
||||
scope="browse",
|
||||
report=_rep,
|
||||
)
|
||||
# A real semantic query competing for a menu slot — logged like the scored
|
||||
# arm it displaces. Before this, the hit it PUSHED OUT was in
|
||||
@@ -433,6 +483,8 @@ async def _reserve_slot_for_reuse(
|
||||
user_id=user_id, source="reuse_slot", query=query,
|
||||
threshold=cfg["threshold"], limit=1, project_id=project_id,
|
||||
is_task=None, results=reuse,
|
||||
best_available=_rep.get("best_available_score"),
|
||||
searched=bool(_rep.get("searched", True)),
|
||||
duration_ms=(time.perf_counter() - _t0) * 1000.0,
|
||||
)
|
||||
# Verify the kind rather than trusting the query that asked for it, and
|
||||
@@ -482,6 +534,7 @@ async def build_autoinject_hint(
|
||||
return empty
|
||||
|
||||
t0 = time.perf_counter()
|
||||
_rep_ai: dict = {}
|
||||
hits = await semantic_search_notes(
|
||||
user_id, q,
|
||||
limit=cfg["top_k"],
|
||||
@@ -493,11 +546,14 @@ async def build_autoinject_hint(
|
||||
# still appear is a collaborator's note inside a shared project — legible
|
||||
# only because the line below names its owner.
|
||||
scope="browse",
|
||||
report=_rep_ai,
|
||||
)
|
||||
record_retrieval(
|
||||
user_id=user_id, source="auto_inject", query=q,
|
||||
threshold=cfg["threshold"], limit=cfg["top_k"],
|
||||
project_id=(project_id or None), is_task=None, results=hits,
|
||||
best_available=_rep_ai.get("best_available_score"),
|
||||
searched=bool(_rep_ai.get("searched", True)),
|
||||
duration_ms=(time.perf_counter() - t0) * 1000.0,
|
||||
)
|
||||
if not hits:
|
||||
@@ -924,6 +980,7 @@ async def build_write_path_hint(
|
||||
# Pulled-and-seen ids stay in the query (as evidence) but never in
|
||||
# the menu — the dedup contract holds, the resemblance still lands.
|
||||
pulled_seen = seen & set(pulled)
|
||||
_rep_wp: dict = {}
|
||||
hits = await semantic_search_notes(
|
||||
user_id, query,
|
||||
limit=remaining + len(pulled_seen),
|
||||
@@ -947,12 +1004,40 @@ async def build_write_path_hint(
|
||||
# Same reasoning as auto-inject: nobody asked for this, so it takes
|
||||
# the browse scope and never surfaces a one-to-one direct share.
|
||||
scope="browse",
|
||||
report=_rep_wp,
|
||||
)
|
||||
resembles = {
|
||||
int(note.id): float(score) for score, note in hits
|
||||
if int(note.id) in pulled
|
||||
}
|
||||
hits = [(s, n) for s, n in hits if int(n.id) not in seen][:remaining]
|
||||
shown = [(s, n) for s, n in hits if int(n.id) not in seen]
|
||||
# WHAT THIS ARM WITHHELD AFTER THE SEARCH ANSWERED, and the reason
|
||||
# `best_available_score` cannot always be reported here (#3739 again,
|
||||
# from the side its fix did not reach).
|
||||
#
|
||||
# This arm is the one note arm that filters TWICE. `exclude_ids` takes
|
||||
# `seen - pulled_seen` into the search, but the pulled-and-seen ids stay
|
||||
# in the query deliberately — `resembles` above needs them — and are
|
||||
# dropped in the line above instead. So the score the search reported is
|
||||
# PRE that drop while the row's `result_count` is POST it, and a record
|
||||
# the session had already been shown could be logged as something the
|
||||
# BAR turned away. Live proof on the first read after #3739 shipped:
|
||||
# write_path's near-miss max was 0.822 while the lowest score it ever
|
||||
# RETURNED was 0.6857 — a "rejection" that beat every acceptance.
|
||||
#
|
||||
# The suppression column cannot rescue it the way it does for the rule
|
||||
# arms: this arm's count would be PARTIAL, covering only the drops made
|
||||
# here and not the ones `exclude_ids` made inside the search, and a
|
||||
# partial number under a name that reads as complete is the substitution
|
||||
# this whole milestone exists to stop.
|
||||
#
|
||||
# So the honest answer is null — "not measured on this call" — whenever
|
||||
# this filter removed anything, because then the bar is not the only
|
||||
# thing that turned something away and the reported score may belong to
|
||||
# a record we withheld ourselves. Calls where nothing was dropped keep
|
||||
# reporting it, which is most of them.
|
||||
withheld_here = len(hits) - len(shown)
|
||||
hits = shown[:remaining]
|
||||
record_retrieval(
|
||||
user_id=user_id, source="write_path", query=query,
|
||||
threshold=cfg["threshold"], limit=remaining,
|
||||
@@ -960,6 +1045,10 @@ async def build_write_path_hint(
|
||||
# recording it as a notes-only retrieval would misdescribe the
|
||||
# candidate set the threshold is being tuned against.
|
||||
project_id=scope_project, is_task=None, results=hits,
|
||||
best_available=(
|
||||
None if withheld_here else _rep_wp.get("best_available_score")
|
||||
),
|
||||
searched=bool(_rep_wp.get("searched", True)),
|
||||
duration_ms=(time.perf_counter() - t0) * 1000.0,
|
||||
)
|
||||
if hits:
|
||||
@@ -1203,9 +1292,11 @@ async def build_write_path_hint(
|
||||
# — a gap that reads as "this surface is somehow not measurable" rather
|
||||
# than "nobody passed the number".
|
||||
rule_t0 = time.perf_counter()
|
||||
_rep_wpr: dict = {}
|
||||
hits = await semantic_search_rules(
|
||||
user_id, code or path, limit=RULEHINT_LIMIT,
|
||||
threshold=cfg["rule_threshold"], tier="conditional",
|
||||
threshold=cfg["rule_threshold"],
|
||||
report=_rep_wpr,
|
||||
)
|
||||
rule_ms = (time.perf_counter() - rule_t0) * 1000.0
|
||||
fresh = [(score, rule) for score, rule in hits if rule.id not in already]
|
||||
@@ -1253,6 +1344,8 @@ async def build_write_path_hint(
|
||||
threshold=cfg["rule_threshold"], limit=RULEHINT_LIMIT,
|
||||
project_id=project_id,
|
||||
is_task=None, results=fresh, duration_ms=rule_ms,
|
||||
best_available=_rep_wpr.get("best_available_score"),
|
||||
searched=bool(_rep_wpr.get("searched", True)),
|
||||
# What the ranker found and this session had already been told.
|
||||
# Without it a zero row cannot say whether the bar was too high or
|
||||
# the reader was simply ahead of it — and only the first is a
|
||||
@@ -1335,9 +1428,11 @@ async def build_tool_rule_hint(
|
||||
query = command[:_TOOL_QUERY_CHARS]
|
||||
|
||||
t0 = time.perf_counter()
|
||||
_rep_ptr: dict = {}
|
||||
hits = await semantic_search_rules(
|
||||
user_id, query, limit=RULEHINT_LIMIT,
|
||||
threshold=cfg["rule_threshold"], tier="conditional",
|
||||
threshold=cfg["rule_threshold"],
|
||||
report=_rep_ptr,
|
||||
)
|
||||
duration_ms = (time.perf_counter() - t0) * 1000.0
|
||||
|
||||
@@ -1357,6 +1452,8 @@ async def build_tool_rule_hint(
|
||||
threshold=cfg["rule_threshold"], limit=RULEHINT_LIMIT,
|
||||
project_id=project_id,
|
||||
is_task=None, results=fresh, duration_ms=duration_ms,
|
||||
best_available=_rep_ptr.get("best_available_score"),
|
||||
searched=bool(_rep_ptr.get("searched", True)),
|
||||
# See the sibling arm. It matters more here: this arm fires on every
|
||||
# Bash call, so a long session excludes its way to an all-zero row
|
||||
# and the threshold looks wrong when nothing about it is.
|
||||
|
||||
@@ -56,6 +56,7 @@ def _build_payload(
|
||||
results: list[tuple[float, Note]],
|
||||
duration_ms: float | None,
|
||||
suppressed: int | None = None,
|
||||
best_available: float | None = None,
|
||||
) -> dict:
|
||||
"""Reduce a retrieval call to a flat, JSON-safe RetrievalLog payload.
|
||||
|
||||
@@ -67,6 +68,13 @@ def _build_payload(
|
||||
had already been shown them, and it stays None for callers that cannot
|
||||
know. See the column's comment: None means "not measured here", which is a
|
||||
different fact from 0 and must never render as one.
|
||||
|
||||
`best_available` is the highest score the ranker reached BEFORE the
|
||||
threshold, and it carries the same null discipline for a sharper reason: it
|
||||
is the only field that still says something on a call that returned
|
||||
nothing, so a 0.0 standing in for "not measured" would read as "the corpus
|
||||
held nothing remotely relevant" — a claim about the corpus invented out of
|
||||
a caller's silence.
|
||||
"""
|
||||
items = [
|
||||
{"id": int(note.id), "score": round(float(score), 5), "rank": rank}
|
||||
@@ -85,6 +93,9 @@ def _build_payload(
|
||||
"suppressed_count": (None if suppressed is None else int(suppressed)),
|
||||
"top_score": (scores[0] if scores else None),
|
||||
"min_score": (scores[-1] if scores else None),
|
||||
"best_available_score": (
|
||||
None if best_available is None else round(float(best_available), 5)
|
||||
),
|
||||
"result_ids": items,
|
||||
"duration_ms": (round(duration_ms, 2) if duration_ms is not None else None),
|
||||
}
|
||||
@@ -123,6 +134,8 @@ def record_retrieval(
|
||||
results: list[tuple[float, Any]],
|
||||
duration_ms: float | None = None,
|
||||
suppressed: int | None = None,
|
||||
best_available: float | None = None,
|
||||
searched: bool = True,
|
||||
) -> None:
|
||||
"""Fire-and-forget: record one retrieval call.
|
||||
|
||||
@@ -134,9 +147,35 @@ def record_retrieval(
|
||||
provide. retrieval_logs is not restored at all, so it has no such hazard,
|
||||
and `source` already distinguishes the surfaces.
|
||||
|
||||
`searched=False` WRITES NO ROW, and that is the point rather than an
|
||||
optimisation. A semantic search has three ways to return nothing without
|
||||
having run — an empty query, an unavailable embedder, and the broad
|
||||
`except` around the query itself — and each one currently arrives here
|
||||
looking exactly like a ranker that declined. Logging it would report a
|
||||
decline nobody made, drag `zero_result_calls` down with phantom evidence
|
||||
about a threshold, and leave `best_available_score` null for a reason that
|
||||
has nothing to do with the corpus. That last ambiguity is #3765: the field
|
||||
added to judge a bar was null on four unrelated causes, one of them a
|
||||
swallowed failure, and no reader could tell them apart.
|
||||
Dropping the row is what makes the remaining nulls mean ONE thing —
|
||||
"searched, and there was nothing".
|
||||
|
||||
The same convention already governs the pre-tool arm: a blank command costs
|
||||
no embedding query, so it writes no row, because "a row here would report a
|
||||
call that never happened and drag the clear-rate down with phantom
|
||||
declines". This extends it from a case the caller could see in advance to
|
||||
the ones only the search knows about.
|
||||
|
||||
A FAILURE IS NOT MADE INVISIBLE BY THIS. `semantic_search_notes` logs a
|
||||
WARNING on a query failure, which is where a broken search belongs — a
|
||||
counter cannot say "I am broken" without a reader already trusting it.
|
||||
|
||||
Builds the payload inline (synchronously) then schedules the insert so the
|
||||
caller returns immediately. Never raises — telemetry must not affect search.
|
||||
"""
|
||||
if not searched:
|
||||
return
|
||||
|
||||
try:
|
||||
payload = _build_payload(
|
||||
user_id=user_id,
|
||||
@@ -149,6 +188,7 @@ def record_retrieval(
|
||||
results=results,
|
||||
duration_ms=duration_ms,
|
||||
suppressed=suppressed,
|
||||
best_available=best_available,
|
||||
)
|
||||
except Exception:
|
||||
logger.debug("retrieval telemetry payload build failed", exc_info=True)
|
||||
@@ -175,19 +215,25 @@ def record_retrieval(
|
||||
|
||||
def _bucket(rows: list) -> dict:
|
||||
"""A score readout a human can act on, from one aggregate row."""
|
||||
(calls, zero, cleared, p10, p50, p90, lo, hi, avg_n, dur,
|
||||
measured, supp_calls, supp_zero) = rows
|
||||
(calls, zero, p10, p50, p90, lo, hi, avg_n, dur,
|
||||
measured, supp_calls, supp_zero,
|
||||
miss_calls, miss_p50, miss_p90, miss_max) = rows
|
||||
return {
|
||||
"calls": int(calls or 0),
|
||||
# A call that returned nothing is not a low-scoring call — it is a
|
||||
# different failure (nothing indexed, filter too narrow), and averaging
|
||||
# it into the score distribution would hide both.
|
||||
"zero_result_calls": int(zero or 0),
|
||||
# How often the best hit actually cleared the threshold in force for
|
||||
# that call. THE precision-adjacent number: a surface that clears its
|
||||
# bar on almost every call is either well-tuned or too loose, and the
|
||||
# score spread below says which.
|
||||
"cleared_threshold": int(cleared or 0),
|
||||
# `cleared_threshold` USED TO LIVE HERE and it was a tautology (#3670).
|
||||
# The search applies the bar before returning, so every returned result
|
||||
# cleared it by construction and a call with nothing has no score to
|
||||
# compare — the condition was true exactly when `result_count > 0`.
|
||||
# `zero_result_calls + cleared_threshold == calls` held on all nineteen
|
||||
# readings ever taken. It was `calls - zero_result_calls` wearing a name
|
||||
# that promised a second opinion, and the docstring built a reading
|
||||
# procedure on it that asked the reader to compare a number with itself.
|
||||
# Its replacement is `near_misses` below, which the bar cannot fix by
|
||||
# construction because it is measured on the calls the bar REJECTED.
|
||||
# Of the zeros above, which were the RANKER declining and which were
|
||||
# the reader having seen it already? `zero_result_calls` cannot say,
|
||||
# and only the first kind is evidence about the threshold.
|
||||
@@ -208,15 +254,99 @@ def _bucket(rows: list) -> dict:
|
||||
"p10": _round(p10), "p50": _round(p50), "p90": _round(p90),
|
||||
"min": _round(lo), "max": _round(hi),
|
||||
},
|
||||
# WHAT THE BAR TURNED AWAY, and the only figure here a threshold can
|
||||
# actually be tuned from. Measured over the calls that returned
|
||||
# NOTHING, on the best score the ranker reached before the filter.
|
||||
#
|
||||
# Read `p90` against the threshold in force. A bar at 0.72 rejecting a
|
||||
# stream of 0.71s is set too high by a hair and the surface is losing
|
||||
# hits it should have had; the same bar rejecting 0.30s is doing its
|
||||
# job and the corpus simply had nothing. Both render as a zero-result
|
||||
# call, and nothing else in this readout separates them.
|
||||
#
|
||||
# None — not a zeroed block — when no declining call in the window
|
||||
# measured it. Old rows predate the column, and a 0.0 would assert that
|
||||
# the corpus held nothing relevant, which is a claim about the corpus
|
||||
# invented out of a caller's silence.
|
||||
"near_misses": (
|
||||
None if not int(miss_calls or 0) else {
|
||||
"measured_calls": int(miss_calls or 0),
|
||||
"p50": _round(miss_p50),
|
||||
"p90": _round(miss_p90),
|
||||
"max": _round(miss_max),
|
||||
}
|
||||
),
|
||||
"avg_result_count": _round(avg_n),
|
||||
"p90_duration_ms": _round(dur, 1),
|
||||
}
|
||||
|
||||
|
||||
# The aggregate row Postgres would have returned for a source with no rows in
|
||||
# the window: nothing counted, nothing scored. Positional, matching the SELECT
|
||||
# `_bucket` unpacks — calls, zero, p10, p50, p90, min, max, avg_n,
|
||||
# dur, measured, supp_calls, supp_zero, miss_calls, miss_p50, miss_p90,
|
||||
# miss_max. The counts are 0 because zero calls is a real observation;
|
||||
# everything else is None because a distribution nobody sampled has no value,
|
||||
# and rendering it as 0.0 would state one.
|
||||
_NO_ROWS_IN_WINDOW = [0, 0, None, None, None, None, None, None, None,
|
||||
0, 0, 0, 0, None, None, None]
|
||||
|
||||
|
||||
def _round(v, places: int = 4):
|
||||
return None if v is None else round(float(v), places)
|
||||
|
||||
|
||||
async def _complete_from(session, model, user_id) -> dict[str, Any]:
|
||||
"""When each source in `model` started being recorded, and the instant the
|
||||
WHOLE table is complete from. Returns {source: earliest_row, "*": latest}.
|
||||
|
||||
THE GRAIN IS THE SOURCE, and that is the whole point. `retrieval_logs` has
|
||||
rows going back months, so a table-level "earliest row" says months and
|
||||
tells a reader their window is fully covered — while a source added last
|
||||
week has a week of rows and a counter that silently means something else.
|
||||
Per-source is the only grain at which partial coverage is visible.
|
||||
|
||||
THE AGGREGATE USES THE LATEST, NOT THE EARLIEST. A number that sums several
|
||||
sources is complete only once EVERY contributor was recording, so "*" is a
|
||||
max over the sources, not a min. Taking the min here would reproduce the
|
||||
exact reading this exists to prevent: the oldest source vouching for the
|
||||
youngest.
|
||||
|
||||
All-time, deliberately unfiltered by the window — a query bounded by
|
||||
`since` can only ever report something at or after `since`, which answers
|
||||
nothing.
|
||||
"""
|
||||
rows = (
|
||||
await session.execute(
|
||||
select(model.source, func.min(model.created_at))
|
||||
.where(model.user_id == user_id)
|
||||
.group_by(model.source)
|
||||
)
|
||||
).all()
|
||||
out: dict[str, Any] = {src: ts for src, ts in rows if ts is not None}
|
||||
stamps = list(out.values())
|
||||
out["*"] = max(stamps) if stamps else None
|
||||
return out
|
||||
|
||||
|
||||
def _coverage(complete_from, since) -> dict:
|
||||
"""The two keys every counter block carries, from one timestamp.
|
||||
|
||||
`covers_window` is None — never False — when nothing was ever recorded.
|
||||
"No rows at all" is not "partial coverage", it is no measurement, and the
|
||||
null convention #3497 established for `suppression` holds here for the
|
||||
same reason: absent must not read as a verdict.
|
||||
"""
|
||||
return {
|
||||
# iso() already returns None for an unset value (#2845) — the guard
|
||||
# belongs on covers_window, which is a verdict, not a serialisation.
|
||||
"complete_from": iso(complete_from),
|
||||
"covers_window": (
|
||||
None if complete_from is None else complete_from <= since
|
||||
),
|
||||
}
|
||||
|
||||
|
||||
async def retrieval_summary(user_id: int | None, *, days: int = 30) -> dict:
|
||||
"""What the retrieval telemetry says, per surface, over a window.
|
||||
|
||||
@@ -256,16 +386,52 @@ async def retrieval_summary(user_id: int | None, *, days: int = 30) -> dict:
|
||||
"read_failed": False,
|
||||
}
|
||||
|
||||
cleared = case(
|
||||
(
|
||||
(RetrievalLog.threshold.isnot(None))
|
||||
& (RetrievalLog.top_score.isnot(None))
|
||||
& (RetrievalLog.top_score >= RetrievalLog.threshold),
|
||||
1,
|
||||
),
|
||||
else_=0,
|
||||
)
|
||||
zero = case((RetrievalLog.result_count == 0, 1), else_=0)
|
||||
# THE NEAR-MISS POPULATION: calls that returned nothing BECAUSE THE BAR
|
||||
# TURNED SOMETHING AWAY, and recorded what it was. Three conditions, and
|
||||
# the third was missing for one deploy (#3739).
|
||||
#
|
||||
# Zero-result only: on a call that returned something,
|
||||
# `best_available_score` equals `top_score` and adds nothing.
|
||||
#
|
||||
# Non-null only: rows written before #3670 genuinely do not know, and must
|
||||
# not read as scoreless declines.
|
||||
#
|
||||
# AND NOT A REPEAT. A zero-result call is two unrelated events — the ranker
|
||||
# found nothing above the bar, or it found only what this session had
|
||||
# already been shown — and just the first says anything about the bar. That
|
||||
# is the whole of #3497, and #3670 reintroduced the conflation one level up:
|
||||
# the rule arms filter exclusions in PYTHON, after the search, so a rule
|
||||
# that cleared the bar and was dropped as a repeat still reported a high
|
||||
# `best_available_score` on a zero-result row. Live proof, first read after
|
||||
# deploy: pre_tool_rule's near-miss max was 0.7457 while the lowest score it
|
||||
# ever RETURNED was 0.7204 — a "rejection" that outscored acceptances.
|
||||
#
|
||||
# The NULL arm is principled, not permissive: `suppressed_count IS NULL`
|
||||
# means the caller passed its exclusions INTO the search, which is exactly
|
||||
# the case where the reported score is already post-exclusion and cannot be
|
||||
# contaminated. Note arms stay measured; rule arms get cleaned.
|
||||
#
|
||||
# Deliberately conservative: a call carrying both a repeat and a lower
|
||||
# genuine miss is dropped whole, losing that point. It undercounts; it
|
||||
# cannot corrupt — the right way round for a number read against a bar.
|
||||
#
|
||||
# This also makes `near_misses.max < threshold` true BY CONSTRUCTION. An
|
||||
# above-bar candidate that was not excluded would have been returned, so
|
||||
# its call is not in this population at all.
|
||||
declined = (
|
||||
(RetrievalLog.result_count == 0)
|
||||
& (RetrievalLog.best_available_score.isnot(None))
|
||||
& (
|
||||
RetrievalLog.suppressed_count.is_(None)
|
||||
| (RetrievalLog.suppressed_count == 0)
|
||||
)
|
||||
)
|
||||
miss = case((declined, 1), else_=0)
|
||||
# `best_available_score` only for those rows; NULL elsewhere, and
|
||||
# percentile_cont ignores NULLs, so the distribution is over the declines
|
||||
# alone without a second pass over the table.
|
||||
miss_score = case((declined, RetrievalLog.best_available_score), else_=None)
|
||||
# Three sums rather than one, because "not measured" and "measured as zero"
|
||||
# are different answers and a single counter cannot hold both.
|
||||
measured = case((RetrievalLog.suppressed_count.isnot(None), 1), else_=0)
|
||||
@@ -283,6 +449,9 @@ async def retrieval_summary(user_id: int | None, *, days: int = 30) -> dict:
|
||||
by_source_rows = None
|
||||
rule_rows = None
|
||||
distinct_rules_surfaced = distinct_rules_pulled = 0
|
||||
# None means the coverage read did not happen — distinct from a table with
|
||||
# no rows, which is {"*": None}. Same reason `read_failed` exists.
|
||||
note_complete = rule_complete = None
|
||||
|
||||
try:
|
||||
async with async_session() as session:
|
||||
@@ -292,7 +461,6 @@ async def retrieval_summary(user_id: int | None, *, days: int = 30) -> dict:
|
||||
RetrievalLog.source,
|
||||
func.count().label("calls"),
|
||||
func.sum(zero).label("zero"),
|
||||
func.sum(cleared).label("cleared"),
|
||||
pct(0.1), pct(0.5), pct(0.9),
|
||||
func.min(RetrievalLog.top_score),
|
||||
func.max(RetrievalLog.top_score),
|
||||
@@ -303,6 +471,10 @@ async def retrieval_summary(user_id: int | None, *, days: int = 30) -> dict:
|
||||
func.sum(measured).label("measured"),
|
||||
func.sum(supp_calls).label("supp_calls"),
|
||||
func.sum(supp_zero).label("supp_zero"),
|
||||
func.sum(miss).label("miss_calls"),
|
||||
func.percentile_cont(0.5).within_group(miss_score.asc()),
|
||||
func.percentile_cont(0.9).within_group(miss_score.asc()),
|
||||
func.max(miss_score),
|
||||
)
|
||||
.where(
|
||||
RetrievalLog.created_at >= since,
|
||||
@@ -311,8 +483,34 @@ async def retrieval_summary(user_id: int | None, *, days: int = 30) -> dict:
|
||||
.group_by(RetrievalLog.source)
|
||||
)
|
||||
).all()
|
||||
log_complete = await _complete_from(session, RetrievalLog, user_id)
|
||||
for row in rows:
|
||||
out["sources"][row[0]] = _bucket(list(row[1:]))
|
||||
source = row[0]
|
||||
bucket = _bucket(list(row[1:]))
|
||||
# Per SOURCE, not per table: retrieval_logs goes back months
|
||||
# while any individual arm may be days old, and the table's
|
||||
# age would vouch for an arm that has barely started.
|
||||
bucket.update(_coverage(log_complete.get(source), since))
|
||||
out["sources"][source] = bucket
|
||||
|
||||
# A source with rows in the table but NONE in this window would
|
||||
# otherwise be absent from the readout — and absent is exactly how
|
||||
# a source that never existed renders, so a surface that WAS
|
||||
# recording and went silent is unreadable (#3720). That is #2663
|
||||
# one level up: the failure that looks like the correct answer.
|
||||
#
|
||||
# Zero here is a real measurement, not a manufactured one. The
|
||||
# all-time query proves the source was recording, and it made no
|
||||
# calls across a window it fully covers — which is why no
|
||||
# `covers_window` special case is needed: a source whose first row
|
||||
# fell after `since` would have that row IN the window and already
|
||||
# hold a bucket, so anything reaching here began before it.
|
||||
for src, first_row in log_complete.items():
|
||||
if src == "*" or first_row is None or src in out["sources"]:
|
||||
continue
|
||||
quiet = _bucket(list(_NO_ROWS_IN_WINDOW))
|
||||
quiet.update(_coverage(first_row, since))
|
||||
out["sources"][src] = quiet
|
||||
|
||||
# The corpus side, at its own grain. `ambient` mirrors
|
||||
# note_usage.usage_for_notes: an ambient surfacing was not a scored
|
||||
@@ -339,6 +537,7 @@ async def retrieval_summary(user_id: int | None, *, days: int = 30) -> dict:
|
||||
.group_by(NoteUsageEvent.event, NoteUsageEvent.source)
|
||||
)
|
||||
).all()
|
||||
note_complete = await _complete_from(session, NoteUsageEvent, user_id)
|
||||
|
||||
# Distinct-note counts need their OWN queries, and this is not
|
||||
# fussiness: count(distinct note_id) per (event, source) group
|
||||
@@ -466,6 +665,9 @@ async def retrieval_summary(user_id: int | None, *, days: int = 30) -> dict:
|
||||
.group_by(RuleUsageEvent.event, RuleUsageEvent.source)
|
||||
)
|
||||
).all()
|
||||
rule_complete = await _complete_from(
|
||||
session, RuleUsageEvent, user_id,
|
||||
)
|
||||
# The rows carry `source`, so the ranked/ambient split is done
|
||||
# below rather than in SQL — the bulk surfaces started emitting
|
||||
# on 2026-09-03 (#3473), so there IS an ambient class now.
|
||||
@@ -575,6 +777,10 @@ async def retrieval_summary(user_id: int | None, *, days: int = 30) -> dict:
|
||||
}
|
||||
usage["by_source"] = by_source
|
||||
|
||||
# The SECTION's coverage, from the latest source to start recording — a
|
||||
# figure that sums several sources is complete only once every one of them
|
||||
# was being written. `_complete_from` computes that as "*".
|
||||
usage.update(_coverage((note_complete or {}).get("*"), since))
|
||||
out["usage"] = usage
|
||||
|
||||
# ── Rules, deliberately a SEPARATE block ────────────────────────────
|
||||
@@ -652,6 +858,7 @@ async def retrieval_summary(user_id: int | None, *, days: int = 30) -> dict:
|
||||
round(rule_usage["pulled_by_agent"] / rule_usage["surfaced"], 4)
|
||||
if rule_usage["surfaced"] else None
|
||||
)
|
||||
rule_usage.update(_coverage((rule_complete or {}).get("*"), since))
|
||||
out["rule_usage"] = rule_usage
|
||||
|
||||
return out
|
||||
|
||||
@@ -195,6 +195,46 @@ def test_displaced_topics_live_on_a_delivered_surface():
|
||||
)
|
||||
|
||||
|
||||
# The SECOND pull (milestone 333 step 3). `list_always_on_rules()` fetches the
|
||||
# resident tier; this one says that tier is not all of them, and that a
|
||||
# conditional rule has to be gone looking for. However a surface words the
|
||||
# surrounding prose, it names the call.
|
||||
RETRIEVE = 'content_type="rule"'
|
||||
|
||||
|
||||
def test_every_session_start_surface_states_the_conditional_retrieval():
|
||||
"""The push/pull asymmetry, one level in.
|
||||
|
||||
The tests above pin that a session PULLS the resident rules rather than
|
||||
trusting the SessionStart push. This pins the same shape between the two
|
||||
TIERS: an always-on rule is delivered, a conditional one is retrieved, and
|
||||
a surface that states only the first leaves a session reading its loaded
|
||||
set as the whole rulebook.
|
||||
|
||||
That reading is wrong in the direction that costs something. "Nothing was
|
||||
pushed" and "no rule applies" are different claims, and only one of them
|
||||
has been checked — the same asymmetry as #2198, now between tiers instead
|
||||
of between channels.
|
||||
|
||||
It is also what made the always-on tier the only one that worked, on any
|
||||
install rather than this one (rule 115). A rule nothing retrieves has to be
|
||||
resident to bind at all, so every rule worth keeping becomes resident; and
|
||||
a resident rule costs tokens in every session forever, so a rulebook that
|
||||
only delivers cannot grow past what one session can hold. Retrieval is what
|
||||
lifts that ceiling — and it only fires if something asks.
|
||||
"""
|
||||
missing = []
|
||||
for path in SESSION_START_SURFACES:
|
||||
if RETRIEVE not in path.read_text():
|
||||
missing.append(str(path.relative_to(ROOT)))
|
||||
assert not missing, (
|
||||
f"these surfaces state the always-on pull but never tell the agent to "
|
||||
f"retrieve a conditional rule ({RETRIEVE}): {missing}. A session that "
|
||||
f"reads its loaded set as the whole rulebook will act on \"I was not "
|
||||
f"told\" as if it meant \"there is no rule\" (milestone 333 step 3)."
|
||||
)
|
||||
|
||||
|
||||
def test_no_surface_names_the_push_without_stating_the_pull():
|
||||
"""The exact shape #2497 took.
|
||||
|
||||
|
||||
@@ -0,0 +1,120 @@
|
||||
"""Both rule-creation tools run the propose-then-approve loop (#3557).
|
||||
|
||||
WHY THIS EXISTS
|
||||
|
||||
Every other gate on `create_rule` and `create_project_rule` is about SHAPE:
|
||||
is this a rule or a process, is it one thing you could violate, is it general
|
||||
enough for a rulebook, is it a near-duplicate. All of those improve a rule
|
||||
someone has already decided to write. None of them asks the prior question —
|
||||
whether the person the rule will bind has agreed to be bound by it.
|
||||
|
||||
That question belongs at the tool, because the tool is the last surface a
|
||||
caller reads before the write, and because the write is less reversible than
|
||||
it looks. The operator's yes is not merely consent; it is the one moment the
|
||||
rule is certainly IN FRONT of them. Afterwards it may not be again for
|
||||
months: a conditional rule is not read aloud at session start, and a
|
||||
project-scoped rule does not appear in an unfiltered `list_rules()` at all.
|
||||
The proposal IS the review, so there had better be one.
|
||||
|
||||
WHY IT IS PHRASED AS A PRACTICE AND NOT A PROHIBITION
|
||||
|
||||
The first cut of this guidance opened "NOT YOURS TO CALL UNPROMPTED." That is
|
||||
the wrong instrument, and the failure it invites is worse than the one it
|
||||
prevents: a caller reading a prohibition stops NOTICING rule-shaped things,
|
||||
rather than noticing them and asking. The wanted behaviour is more proposals,
|
||||
not fewer — spotting that something has hardened into a standing instruction
|
||||
is valuable work, and the only step that was ever missing came after it.
|
||||
|
||||
So the docstrings describe what to DO: propose readily, state four things,
|
||||
close with a question the operator answers in one word. This test is written
|
||||
the same way — it asserts the parts of the loop are present, and has nothing
|
||||
to say about any wording that forbids.
|
||||
|
||||
The fourth element — how the rule would be ENFORCED — is not ceremony. It is
|
||||
the part that sometimes dissolves the rule: a thing a test can assert should
|
||||
be that test, and a rule is what is left when nothing mechanical can hold it.
|
||||
A rulebook grows by default and shrinks only on purpose, so the question that
|
||||
prevents a rule earns more than any question that improves one's wording.
|
||||
|
||||
WHAT THIS PINS, AND WHAT IT DOES NOT
|
||||
|
||||
STRUCTURE, never wording — the same bargain the disambiguator guard (#3123)
|
||||
strikes next door. Each element matches a family of synonyms, so the prose
|
||||
stays free to be rewritten, reordered or sharpened; only DELETING one fails.
|
||||
Pinning phrasing would make every improvement a red build, and a test that
|
||||
punishes editing is a test someone deletes.
|
||||
|
||||
It cannot tell whether an agent actually proposes. Nothing in a docstring
|
||||
can. It catches the regression that really happens: guidance tidied away in
|
||||
a later pass by someone who read it as throat-clearing in front of the Args.
|
||||
"""
|
||||
import pytest
|
||||
|
||||
from tests.helpers import tool_doc as _doc
|
||||
|
||||
# Both surfaces, because the one that needs it most is the one that looks
|
||||
# minor. A project rule is the least visible record the system can hold —
|
||||
# absent from an unfiltered list_rules(), and absent from session start too
|
||||
# whenever it is conditional — so the surface that writes one carries the
|
||||
# larger risk while reading as the smaller act.
|
||||
_SURFACES = [
|
||||
("scribe.mcp.tools.rulebooks", "create_rule"),
|
||||
("scribe.mcp.tools.rulebooks", "create_project_rule"),
|
||||
]
|
||||
|
||||
# The loop, element by element, each as a family of ways to say it. A
|
||||
# docstring satisfies an element by containing ANY member — that is the room
|
||||
# left for rewriting. The families deliberately exclude bare words a
|
||||
# docstring would hold by accident ("why", "how", "reason", "rule"), which
|
||||
# would let the assertion pass on prose that says nothing of the kind.
|
||||
_ELEMENTS = {
|
||||
"the invitation to propose": ("propose", "proposal"),
|
||||
"the operator's approval": ("approve", "approval", "says yes", "a yes"),
|
||||
"the rule's intent": ("intent", "what it changes about how work"),
|
||||
"why it is being proposed now": (
|
||||
"why now", "arose_from_id", "the incident", "prompted it",
|
||||
),
|
||||
"how it would be enforced": ("enforc",),
|
||||
"the answers offered back": ("as written", "talk about it", "discuss"),
|
||||
}
|
||||
|
||||
|
||||
@pytest.mark.parametrize(("module", "name"), _SURFACES)
|
||||
@pytest.mark.parametrize("element", sorted(_ELEMENTS))
|
||||
def test_a_rule_surface_carries_every_part_of_the_proposal_loop(
|
||||
module, name, element
|
||||
):
|
||||
"""Each element of propose → state four things → ask survives."""
|
||||
doc = _doc(module, name).lower()
|
||||
assert any(token in doc for token in _ELEMENTS[element]), (
|
||||
f"{name}'s docstring no longer mentions {element}. A caller reads "
|
||||
f"this immediately before writing a rule that will bind every future "
|
||||
f"session, and the proposal is the one moment that rule is certain to "
|
||||
f"be seen by the operator. Say it in whatever words you like; this "
|
||||
f"guard only checks it is still said. See create_rule's opening."
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(("module", "name"), _SURFACES)
|
||||
def test_the_proposal_loop_comes_before_the_parameter_contract(module, name):
|
||||
"""It has to be read to work, and the Args: block is where reading stops.
|
||||
|
||||
A caller who has decided to make the call skims down to the parameters.
|
||||
Guidance parked below them — or folded into one argument's description —
|
||||
arrives after the decision it was meant to inform, which is the same as
|
||||
not being there.
|
||||
"""
|
||||
doc = _doc(module, name).lower()
|
||||
args_at = doc.find("args:")
|
||||
assert args_at > 0, f"{name}'s docstring has no Args: block"
|
||||
loop_at = min(
|
||||
(doc.find(t) for t in _ELEMENTS["the invitation to propose"]
|
||||
if doc.find(t) >= 0),
|
||||
default=-1,
|
||||
)
|
||||
assert 0 <= loop_at < args_at, (
|
||||
f"{name} introduces the proposal loop at or after its Args: block "
|
||||
f"(loop {loop_at}, args {args_at}). Move it to the opening — a "
|
||||
f"caller who has already decided to write the rule reads the "
|
||||
f"parameters, not the prose under them."
|
||||
)
|
||||
@@ -160,7 +160,15 @@ async def test_the_arm_searches_on_its_OWN_bar_not_the_code_one():
|
||||
kw = search.await_args.kwargs
|
||||
assert kw["threshold"] == 0.81, "the arm is still using the code threshold"
|
||||
assert kw["limit"] == pc.RULEHINT_LIMIT
|
||||
assert kw["tier"] == "conditional"
|
||||
# NO tier filter (#3702). The arms search every rule the caller owns,
|
||||
# because "already in the session" is not the same as "in front of the
|
||||
# reader at the moment it applies" — and relevance is the threshold's
|
||||
# job, not a category's. If this assertion is failing because a tier
|
||||
# argument came back, read the block above RULEHINT_LIMIT first: the
|
||||
# filter may legitimately return, but only carrying a measured reason.
|
||||
assert "tier" not in kw or kw["tier"] is None, (
|
||||
"the arm is filtering the rule corpus by tier again"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -764,3 +772,97 @@ async def test_a_shown_hit_is_not_counted_as_suppressed():
|
||||
assert out["rule_ids"] == [161]
|
||||
assert log.call_args.kwargs["suppressed"] == 1
|
||||
assert len(log.call_args.kwargs["results"]) == 1
|
||||
|
||||
|
||||
# ── The identity that falsified this milestone (#3668) ─────────────────
|
||||
#
|
||||
# `rule_usage.surfaced` == `pre_tool_rule.cleared` + `write_path_rule.cleared`.
|
||||
# Milestone #379 was scoped on a reconstruction that put ~64% of ranked rule
|
||||
# surfacings as never reaching `rule_usage_events`. Five steps were planned
|
||||
# against it. One read of this identity — 17 = 17, then 39 = 39 on a second
|
||||
# window — falsified the whole thing: the gap was two counters that started
|
||||
# recording on different days, not a write path dropping rows.
|
||||
#
|
||||
# So the identity is not a nice-to-have. It is the cheapest true statement
|
||||
# available about this pair of tables, and its absence is what let a magnitude
|
||||
# that merely LOOKED wrong survive a code review and a five-step plan. An
|
||||
# identity that must hold exactly beats a magnitude that looks wrong.
|
||||
#
|
||||
# WHY THE ARM IS THE RIGHT PLACE TO PIN IT, and the readout is not. Inside an
|
||||
# arm, one `fresh` list feeds both recorders in one function, so the counts
|
||||
# cannot legitimately differ — at any limit. The readout-level form is weaker
|
||||
# than it looks: `cleared_threshold` counts CALLS that beat the bar while
|
||||
# `surfaced` counts RULES, and those coincide only while `RULEHINT_LIMIT` is 1.
|
||||
# Raise the limit and the readout identity breaks while nothing is wrong.
|
||||
# `RULEHINT_LIMIT` has already moved once (2 → 1, `2385100`), and that move is
|
||||
# half of why the original reconstruction misread its own numbers.
|
||||
#
|
||||
# Hence three hits below, where production currently returns at most one. The
|
||||
# test is deliberately in a state the limit does not permit today, because what
|
||||
# is being pinned is that the two recorders read the same list — not that the
|
||||
# list happens to be short.
|
||||
|
||||
_THREE_HITS = [
|
||||
(0.81, fake_rule(id=156, title="A wait with no deadline is a bug")),
|
||||
(0.77, fake_rule(id=157, title="A loop re-arms in a finally")),
|
||||
(0.74, fake_rule(id=161, title="Reach the forge through its MCP tools")),
|
||||
]
|
||||
|
||||
_ARMS = [("write_path_rule", _run_arm), ("pre_tool_rule", _run_tool_arm)]
|
||||
|
||||
|
||||
def _both_ends(log, rec, source):
|
||||
"""What the two recorders said about one call, at the same grain.
|
||||
|
||||
Ids rather than counts. Equal counts drawn from different lists is a real
|
||||
way for this to break — an off-by-one slice, or one recorder reading `hits`
|
||||
where the other reads `fresh` in a window where the exclusion happened to
|
||||
remove as many as it added — and a count comparison would call that agreement.
|
||||
"""
|
||||
rows = [c for c in log.call_args_list if c.kwargs.get("source") == source]
|
||||
assert len(rows) == 1, (
|
||||
f"expected exactly one {source} call row, got {len(rows)} — the "
|
||||
f"identity is per call and cannot be read across several"
|
||||
)
|
||||
logged = [rule.id for _score, rule in rows[0].kwargs["results"]]
|
||||
surfaced = [
|
||||
rid
|
||||
for c in rec.call_args_list if c.kwargs.get("source") == source
|
||||
for rid in c.kwargs["rule_ids"]
|
||||
]
|
||||
return logged, surfaced
|
||||
|
||||
|
||||
@pytest.mark.parametrize(("source", "run"), _ARMS, ids=["write_path", "pre_tool"])
|
||||
@pytest.mark.parametrize(
|
||||
("excluded", "expected"),
|
||||
[([], 3), ([157], 2), ([156, 157, 161], 0)],
|
||||
ids=["nothing-held", "one-already-held", "all-already-held"],
|
||||
)
|
||||
@pytest.mark.asyncio
|
||||
async def test_both_recorders_report_the_same_rules_for_one_call(
|
||||
source, run, excluded, expected
|
||||
):
|
||||
"""One list, two tables, no room to disagree.
|
||||
|
||||
The middle case is the one that discriminates. With nothing excluded both
|
||||
recorders see the same three rules however wrongly they are wired, so an
|
||||
arm logging `hits` to the call log and `fresh` to the surfacing log passes
|
||||
that case and fails this one — and logging `hits` is exactly the divergence
|
||||
that would manufacture an apparent write loss out of a correct system.
|
||||
"""
|
||||
log, rec = MagicMock(), MagicMock()
|
||||
await run(list(_THREE_HITS), rec, retrieval_log=log, exclude_rule_ids=excluded)
|
||||
|
||||
logged, surfaced = _both_ends(log, rec, source)
|
||||
assert surfaced == logged, (
|
||||
f"{source} told its two tables different stories about one call: the "
|
||||
f"call log recorded {logged} and the surfacing log recorded {surfaced}. "
|
||||
f"Both come from `fresh`, in one function, so any difference is a bug "
|
||||
f"in the wiring — and it is the shape that reads as a lost write when "
|
||||
f"the two tables are later compared in aggregate (#3668)."
|
||||
)
|
||||
assert len(logged) == expected, (
|
||||
"the fixture stopped exercising what it claims to; check the exclusion "
|
||||
"filter still runs before both recorders"
|
||||
)
|
||||
|
||||
@@ -102,14 +102,14 @@ def test_the_readout_reports_unmeasured_suppression_as_none():
|
||||
zeroed dict: a zeroed dict states a measurement nobody made."""
|
||||
from scribe.services.retrieval_telemetry import _bucket
|
||||
|
||||
# calls, zero, cleared, p10, p50, p90, min, max, avg_n, dur,
|
||||
# measured, supp_calls, supp_zero
|
||||
unmeasured = _bucket([326, 114, 212, 0.6, 0.68, 0.77, 0.55, 0.85, 1.7, 130.9,
|
||||
0, 0, 0])
|
||||
# calls, zero, p10, p50, p90, min, max, avg_n, dur,
|
||||
# measured, supp_calls, supp_zero, miss_calls, miss_p50, miss_p90, miss_max
|
||||
unmeasured = _bucket([326, 114, 0.6, 0.68, 0.77, 0.55, 0.85, 1.7, 130.9,
|
||||
0, 0, 0, 0, None, None, None])
|
||||
assert unmeasured["suppression"] is None
|
||||
|
||||
measured = _bucket([35, 34, 1, 0.75, 0.75, 0.75, 0.75, 0.75, 0.03, 51.9,
|
||||
35, 9, 9])
|
||||
measured = _bucket([35, 34, 0.75, 0.75, 0.75, 0.75, 0.75, 0.03, 51.9,
|
||||
35, 9, 9, 0, None, None, None])
|
||||
assert measured["suppression"] == {
|
||||
"measured_calls": 35,
|
||||
"calls_with_suppression": 9,
|
||||
@@ -224,7 +224,10 @@ async def test_retrieval_summary_reads_what_the_writer_wrote(_dispose_engine):
|
||||
ai = out["sources"]["auto_inject"]
|
||||
assert ai["calls"] == 4
|
||||
assert ai["zero_result_calls"] == 1
|
||||
assert ai["cleared_threshold"] == 2 # 0.91 and 0.72, not 0.40
|
||||
assert "cleared_threshold" not in ai, (
|
||||
"the tautology is back: it was true exactly when result_count > 0, "
|
||||
"so it reported nothing zero_result_calls did not (#3670)"
|
||||
)
|
||||
# p50 over the three scored calls; the empty one contributes no score.
|
||||
assert ai["top_score"]["p50"] == pytest.approx(0.72, abs=1e-4)
|
||||
assert ai["top_score"]["min"] == pytest.approx(0.40, abs=1e-4)
|
||||
@@ -638,3 +641,482 @@ async def test_ambient_alone_reports_no_ratio(_dispose_engine):
|
||||
assert ru["pull_through"] is None
|
||||
finally:
|
||||
await cleanup()
|
||||
|
||||
|
||||
# ── Window coverage (#3712) ────────────────────────────────────────────
|
||||
#
|
||||
# A counter added last week, read over a 30-day window, reports a real count
|
||||
# against an imagined denominator. The result is a plausible FRACTION rather
|
||||
# than an obvious zero, which is what makes it dangerous — #379 spent five
|
||||
# planned steps on a defect that turned out to be a window opening before the
|
||||
# recording it was measuring existed.
|
||||
|
||||
|
||||
def test_coverage_says_nothing_rather_than_false_when_nothing_was_recorded():
|
||||
"""Null, never False. "No measurement" is not "partial measurement".
|
||||
|
||||
The same distinction `suppression`'s null carries (#3497): absent must not
|
||||
read as a verdict. A False here would assert the window is under-covered,
|
||||
which is a claim nobody is in a position to make.
|
||||
"""
|
||||
from datetime import datetime, timezone
|
||||
|
||||
from scribe.services.retrieval_telemetry import _coverage
|
||||
|
||||
since = datetime(2026, 9, 1, tzinfo=timezone.utc)
|
||||
assert _coverage(None, since) == {
|
||||
"complete_from": None, "covers_window": None,
|
||||
}
|
||||
|
||||
|
||||
def test_coverage_reads_a_start_before_the_window_as_covered():
|
||||
from datetime import datetime, timezone
|
||||
|
||||
from scribe.services.retrieval_telemetry import _coverage
|
||||
|
||||
since = datetime(2026, 9, 1, tzinfo=timezone.utc)
|
||||
older = datetime(2026, 8, 1, tzinfo=timezone.utc)
|
||||
newer = datetime(2026, 9, 5, tzinfo=timezone.utc)
|
||||
|
||||
assert _coverage(older, since)["covers_window"] is True
|
||||
assert _coverage(newer, since)["covers_window"] is False, (
|
||||
"a counter that started inside the window covers only part of it"
|
||||
)
|
||||
assert _coverage(newer, since)["complete_from"] == newer.isoformat()
|
||||
|
||||
|
||||
@pytest.mark.integration
|
||||
@pytest.mark.asyncio
|
||||
async def test_coverage_is_per_source_because_the_table_is_older_than_its_arms(
|
||||
_dispose_engine,
|
||||
):
|
||||
"""THE grain question, and the reason a per-table answer is useless.
|
||||
|
||||
`retrieval_logs` accumulates for months. A table-level "earliest row"
|
||||
therefore says months for every source it holds — including one added
|
||||
days ago whose counter means something quite different. The old source
|
||||
would vouch for the young one, which is exactly the reading this exists
|
||||
to prevent.
|
||||
"""
|
||||
from datetime import datetime, timedelta, timezone
|
||||
|
||||
from sqlalchemy import delete
|
||||
|
||||
from scribe.models import async_session
|
||||
from scribe.models.retrieval_log import RetrievalLog
|
||||
from scribe.services.retrieval_telemetry import retrieval_summary
|
||||
|
||||
UID = 990077
|
||||
now = datetime.now(timezone.utc)
|
||||
async with async_session() as s:
|
||||
# An old surface, recording since well before any window we ask for,
|
||||
# AND still recording inside it. Both rows are needed: `complete_from`
|
||||
# comes from the all-time query, but a source only gets a bucket at all
|
||||
# if it has rows in the window, so the 90-day row alone would leave
|
||||
# nothing to assert on.
|
||||
s.add(RetrievalLog(
|
||||
user_id=UID, source="auto_inject", result_count=1,
|
||||
created_at=now - timedelta(days=90),
|
||||
))
|
||||
s.add(RetrievalLog(
|
||||
user_id=UID, source="auto_inject", result_count=1,
|
||||
created_at=now - timedelta(days=1),
|
||||
))
|
||||
# A young arm, first written INSIDE the window below.
|
||||
s.add(RetrievalLog(
|
||||
user_id=UID, source="pre_tool_rule", result_count=1,
|
||||
created_at=now - timedelta(days=2),
|
||||
))
|
||||
await s.commit()
|
||||
|
||||
try:
|
||||
out = await retrieval_summary(UID, days=30)
|
||||
|
||||
assert out["sources"]["auto_inject"]["covers_window"] is True
|
||||
assert out["sources"]["pre_tool_rule"]["covers_window"] is False, (
|
||||
"the young arm was reported as covering a 30-day window — the "
|
||||
"table's age has been allowed to vouch for one of its sources"
|
||||
)
|
||||
finally:
|
||||
async with async_session() as s:
|
||||
await s.execute(delete(RetrievalLog).where(RetrievalLog.user_id == UID))
|
||||
await s.commit()
|
||||
|
||||
|
||||
@pytest.mark.integration
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_surface_that_went_silent_is_not_the_same_as_one_that_never_ran(
|
||||
_dispose_engine,
|
||||
):
|
||||
"""#3720 — absent is how "never existed" renders, so it cannot also be how
|
||||
"stopped recording" renders.
|
||||
|
||||
A surface losing its recorder is one of the failures this milestone exists
|
||||
to make visible, and dropping it from the readout is the most complete way
|
||||
to hide it. Zero here is a real measurement: the table proves the source
|
||||
was recording, and it made no calls across a window it fully covers.
|
||||
"""
|
||||
from datetime import datetime, timedelta, timezone
|
||||
|
||||
from sqlalchemy import delete
|
||||
|
||||
from scribe.models import async_session
|
||||
from scribe.models.retrieval_log import RetrievalLog
|
||||
from scribe.services.retrieval_telemetry import retrieval_summary
|
||||
|
||||
UID = 990078
|
||||
now = datetime.now(timezone.utc)
|
||||
async with async_session() as s:
|
||||
# Recorded once, well before the window, and never since.
|
||||
s.add(RetrievalLog(
|
||||
user_id=UID, source="auto_inject", result_count=3, top_score=0.81,
|
||||
created_at=now - timedelta(days=60),
|
||||
))
|
||||
await s.commit()
|
||||
|
||||
try:
|
||||
out = await retrieval_summary(UID, days=7)
|
||||
|
||||
assert "auto_inject" in out["sources"], (
|
||||
"a source with rows in the table but none in the window was "
|
||||
"dropped from the readout — a surface that stopped recording now "
|
||||
"reads exactly like one that never existed"
|
||||
)
|
||||
quiet = out["sources"]["auto_inject"]
|
||||
assert quiet["calls"] == 0
|
||||
# The window IS covered; what was observed across it is nothing.
|
||||
assert quiet["covers_window"] is True
|
||||
# ...but nothing was sampled, so no distribution may be claimed. A
|
||||
# zeroed score would assert a measurement, which is #3311's mistake.
|
||||
assert quiet["top_score"] == {
|
||||
"p10": None, "p50": None, "p90": None, "min": None, "max": None,
|
||||
}
|
||||
assert quiet["suppression"] is None
|
||||
assert quiet["avg_result_count"] is None
|
||||
finally:
|
||||
async with async_session() as s:
|
||||
await s.execute(delete(RetrievalLog).where(RetrievalLog.user_id == UID))
|
||||
await s.commit()
|
||||
|
||||
|
||||
@pytest.mark.integration
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_section_is_complete_only_from_its_latest_contributor(
|
||||
_dispose_engine,
|
||||
):
|
||||
"""A sum is complete once EVERY contributor was being written — so the
|
||||
section takes the LATEST first-row, not the earliest.
|
||||
|
||||
Taking the earliest would be worse than reporting nothing: it would pick
|
||||
the oldest source in the table and use it to certify a total that a
|
||||
newer source is still only partly contributing to. That is the original
|
||||
error in miniature.
|
||||
"""
|
||||
from datetime import datetime, timedelta, timezone
|
||||
|
||||
from sqlalchemy import delete
|
||||
|
||||
from scribe.models import async_session
|
||||
from scribe.models.rule_usage import RuleUsageEvent
|
||||
from scribe.services.retrieval_telemetry import retrieval_summary
|
||||
|
||||
UID = 990078
|
||||
now = datetime.now(timezone.utc)
|
||||
old = now - timedelta(days=90)
|
||||
young = now - timedelta(days=2)
|
||||
async with async_session() as s:
|
||||
s.add_all([
|
||||
RuleUsageEvent(
|
||||
user_id=UID, rule_id=1, event="surfaced",
|
||||
source="list_always_on_rules", created_at=old,
|
||||
),
|
||||
RuleUsageEvent(
|
||||
user_id=UID, rule_id=2, event="surfaced",
|
||||
source="pre_tool_rule", created_at=young,
|
||||
),
|
||||
])
|
||||
await s.commit()
|
||||
|
||||
try:
|
||||
out = await retrieval_summary(UID, days=30)
|
||||
ru = out["rule_usage"]
|
||||
|
||||
assert ru["complete_from"] == young.isoformat(), (
|
||||
"the section reported completeness from its OLDEST source; a "
|
||||
"total is only as complete as its newest contributor"
|
||||
)
|
||||
assert ru["covers_window"] is False
|
||||
finally:
|
||||
async with async_session() as s:
|
||||
await s.execute(delete(RuleUsageEvent).where(RuleUsageEvent.user_id == UID))
|
||||
await s.commit()
|
||||
|
||||
|
||||
# ─── the bar can only be judged from what it rejected (#3670) ────────────────
|
||||
#
|
||||
# `cleared_threshold` was the number the docstring told a reader to look at
|
||||
# first. It was `calls - zero_result_calls` under another name: the search
|
||||
# applies the bar before returning, so every returned result cleared it by
|
||||
# construction and a call with nothing has no score to compare.
|
||||
# `zero_result_calls + cleared_threshold == calls` held on all nineteen
|
||||
# source/window readings ever taken — no near-misses, no exceptions.
|
||||
#
|
||||
# What replaced it cannot go the same way, and the reason is structural rather
|
||||
# than careful naming: `near_misses` is measured on the calls the bar TURNED
|
||||
# AWAY, using a score the bar never saw. No arrangement of `calls`,
|
||||
# `zero_result_calls` and `result_count` derives it.
|
||||
|
||||
|
||||
def test_a_call_that_returned_nothing_still_records_what_it_nearly_showed():
|
||||
"""The whole point, at the payload grain.
|
||||
|
||||
This is the row a threshold is tuned from and the one that used to carry no
|
||||
score at all: `top_score` and `min_score` are both null here, correctly, and
|
||||
a reader was left unable to tell a bar rejecting 0.71s from one rejecting
|
||||
0.30s. Both render as a zero-result call.
|
||||
"""
|
||||
p = _build_payload(
|
||||
user_id=1, source="pre_tool_rule", query="git push --force",
|
||||
threshold=0.72, limit=1, project_id=None, is_task=None,
|
||||
results=[], duration_ms=None, best_available=0.7104,
|
||||
)
|
||||
assert p["result_count"] == 0
|
||||
assert p["top_score"] is None, "nothing was shown, so nothing has a top score"
|
||||
assert p["best_available_score"] == 0.7104, (
|
||||
"the losing score was discarded — the only figure that survives a call "
|
||||
"returning nothing, and the only one a bar can be judged from"
|
||||
)
|
||||
|
||||
|
||||
def test_a_caller_that_did_not_measure_the_near_miss_stores_null():
|
||||
"""Null, never 0.0. A zero here reads as "the corpus held nothing remotely
|
||||
relevant" — a claim about the corpus invented out of a caller's silence,
|
||||
which is #3311's substitution in a new field."""
|
||||
p = _build_payload(
|
||||
user_id=1, source="auto_inject", query="q", threshold=0.6,
|
||||
limit=3, project_id=None, is_task=None, results=[], duration_ms=None,
|
||||
)
|
||||
assert p["best_available_score"] is None
|
||||
|
||||
|
||||
def test_the_readout_reports_unmeasured_near_misses_as_none():
|
||||
"""`_bucket`'s half of the same discipline, and the reason it is a block
|
||||
rather than three loose keys: old rows predate the column, so a window can
|
||||
legitimately contain declines nobody measured."""
|
||||
from scribe.services.retrieval_telemetry import _bucket
|
||||
|
||||
# calls, zero, p10, p50, p90, min, max, avg_n, dur,
|
||||
# measured, supp_calls, supp_zero, miss_calls, miss_p50, miss_p90, miss_max
|
||||
none_measured = _bucket([326, 114, 0.6, 0.68, 0.77, 0.55, 0.85, 1.7, 130.9,
|
||||
0, 0, 0, 0, None, None, None])
|
||||
assert none_measured["near_misses"] is None
|
||||
|
||||
measured = _bucket([326, 114, 0.6, 0.68, 0.77, 0.55, 0.85, 1.7, 130.9,
|
||||
0, 0, 0, 114, 0.61, 0.7104, 0.7189])
|
||||
assert measured["near_misses"] == {
|
||||
"measured_calls": 114,
|
||||
"p50": 0.61,
|
||||
"p90": 0.7104,
|
||||
"max": 0.7189,
|
||||
}
|
||||
|
||||
|
||||
def test_the_readout_carries_no_field_derivable_from_its_neighbours():
|
||||
"""The guard that would have caught #3670 on the day it shipped.
|
||||
|
||||
`cleared_threshold` survived because it had its own name and its own
|
||||
docstring paragraph, and nobody added the two numbers beside it. This
|
||||
asserts the identity that held on every reading ever taken — and if a
|
||||
future field reintroduces it under a new name, the sum below is where it
|
||||
shows up.
|
||||
"""
|
||||
from scribe.services.retrieval_telemetry import _bucket
|
||||
|
||||
b = _bucket([326, 114, 0.6, 0.68, 0.77, 0.55, 0.85, 1.7, 130.9,
|
||||
0, 0, 0, 114, 0.61, 0.71, 0.72])
|
||||
derivable = {
|
||||
k for k, v in b.items()
|
||||
if isinstance(v, int) and not isinstance(v, bool)
|
||||
and k not in ("calls", "zero_result_calls")
|
||||
and v == b["calls"] - b["zero_result_calls"]
|
||||
}
|
||||
assert not derivable, (
|
||||
f"{sorted(derivable)} equals calls - zero_result_calls on this row. "
|
||||
f"That is how `cleared_threshold` read for its whole life (#3670): a "
|
||||
f"figure presented as an independent measurement that a reader can "
|
||||
f"compute from the two numbers next to it. Either it is a tautology, "
|
||||
f"or this fixture happens to make it look like one — check which "
|
||||
f"before adding an exemption."
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.integration
|
||||
@pytest.mark.asyncio
|
||||
async def test_the_near_miss_distribution_is_a_query_postgres_accepts(_dispose_engine):
|
||||
"""Integration, and NOT belt-and-braces on the unit tests above.
|
||||
|
||||
`near_misses` is a `percentile_cont(...) WITHIN GROUP` over a CASE
|
||||
expression, inside the same grouped aggregate that already carries four
|
||||
other CASEs. That is within one step of the shape that produced #2663 — a
|
||||
query the database rejected, swallowed by this module's broad `except`, so
|
||||
every counter read zero in production while the writes landed fine and the
|
||||
mocked tests passed. Only a real Postgres can say this parses, and if it
|
||||
does not, the symptom is silence rather than an error.
|
||||
|
||||
The numbers are chosen so a bar at 0.72 is visibly the wrong bar: three
|
||||
declines at 0.70, 0.71 and 0.7189, none of which a reader could see before.
|
||||
"""
|
||||
from sqlalchemy import delete
|
||||
|
||||
from scribe.models import async_session
|
||||
from scribe.models.retrieval_log import RetrievalLog
|
||||
from scribe.services.retrieval_telemetry import (
|
||||
_insert_retrieval_log, retrieval_summary,
|
||||
)
|
||||
|
||||
UID = 990079
|
||||
for best in (0.70, 0.71, 0.7189):
|
||||
await _insert_retrieval_log(_build_payload(
|
||||
user_id=UID, source="pre_tool_rule", query="git push", threshold=0.72,
|
||||
limit=1, project_id=None, is_task=None, results=[],
|
||||
duration_ms=4.0, best_available=best,
|
||||
))
|
||||
# A call that DID show something. Its best-available equals its top score,
|
||||
# so including it would drag the distribution toward the scores the bar
|
||||
# already accepts — the population has to be the declines alone.
|
||||
await _insert_retrieval_log(_build_payload(
|
||||
user_id=UID, source="pre_tool_rule", query="curl", threshold=0.72,
|
||||
limit=1, project_id=None, is_task=None,
|
||||
results=[(0.88, _note(7))], duration_ms=4.0, best_available=0.88,
|
||||
))
|
||||
# An unmeasured decline, standing in for every row written before #3670.
|
||||
await _insert_retrieval_log(_build_payload(
|
||||
user_id=UID, source="pre_tool_rule", query="ls", threshold=0.72,
|
||||
limit=1, project_id=None, is_task=None, results=[], duration_ms=4.0,
|
||||
))
|
||||
# THE CASE WHOSE ABSENCE LET THIS GUARD PASS OVER BROKEN CODE (#3739).
|
||||
# A zero-result call whose zero was a REPEAT, not a rejection: the ranker
|
||||
# cleared the bar at 0.9 and the session had already been shown that rule,
|
||||
# so the arm dropped it in Python after the search. Without the suppression
|
||||
# arm of the predicate this row lands in the near-miss population and drags
|
||||
# `max` to 0.9 — above the very threshold the field is read against.
|
||||
await _insert_retrieval_log(_build_payload(
|
||||
user_id=UID, source="pre_tool_rule", query="git commit", threshold=0.72,
|
||||
limit=1, project_id=None, is_task=None, results=[], duration_ms=4.0,
|
||||
best_available=0.9, suppressed=1,
|
||||
))
|
||||
|
||||
try:
|
||||
out = await retrieval_summary(UID, days=30)
|
||||
assert out["read_failed"] is False, (
|
||||
"the aggregate did not execute — a rejected query here reads as "
|
||||
"zeros everywhere, which is #2663 exactly"
|
||||
)
|
||||
src = out["sources"]["pre_tool_rule"]
|
||||
assert src["calls"] == 6
|
||||
assert src["zero_result_calls"] == 5
|
||||
|
||||
nm = src["near_misses"]
|
||||
assert nm is not None, "the near-miss block did not survive the query"
|
||||
assert nm["measured_calls"] == 3, (
|
||||
"the population is declines the BAR caused, that recorded a score. "
|
||||
"Three qualify. Excluded: the unmeasured row (predates the column, "
|
||||
"not a scoreless decline), the call that showed something (its "
|
||||
"best-available is just its top score), and the REPEAT — a zero "
|
||||
"the reader caused, not the bar (#3739)"
|
||||
)
|
||||
assert nm["max"] == pytest.approx(0.7189, abs=1e-4), (
|
||||
"the closest thing the bar turned away — 0.7189 against a 0.72 "
|
||||
"threshold, which is the reading the whole field exists to give"
|
||||
)
|
||||
assert nm["max"] < 0.72, (
|
||||
"a rejection that outscores the bar is not a rejection. This is "
|
||||
"structural once the suppression arm is in the predicate: an "
|
||||
"above-bar candidate that was not excluded would have been "
|
||||
"RETURNED, so its call cannot be in this population (#3739)"
|
||||
)
|
||||
assert 0.70 <= nm["p50"] <= 0.7189
|
||||
finally:
|
||||
async with async_session() as s:
|
||||
await s.execute(delete(RetrievalLog).where(RetrievalLog.user_id == UID))
|
||||
await s.commit()
|
||||
|
||||
|
||||
# ─── a search that never ran is not a decline (#3765) ────────────────────────
|
||||
#
|
||||
# `best_available_score` was added by #3670 so a bar could be judged from what
|
||||
# it rejected, and it arrived null on four unrelated causes: the corpus offered
|
||||
# nothing, the query was empty, the embedder was down, or the DATABASE QUERY
|
||||
# FAILED. Only the first is a measurement. The fourth is the #2663 shape — a
|
||||
# swallowed failure rendering as a clean zero — inside the field added to fix
|
||||
# an instance of the #2663 shape.
|
||||
#
|
||||
# The fix is not a new column. A call that never searched writes no row, so
|
||||
# every remaining null means one thing. That is the convention the pre-tool arm
|
||||
# already follows for a blank command, extended from the case a caller can see
|
||||
# in advance to the ones only the search knows about.
|
||||
|
||||
|
||||
def test_a_search_that_never_ran_writes_no_row():
|
||||
"""The whole fix, at the one place it is enforced.
|
||||
|
||||
`record_retrieval` is fire-and-forget and returns None either way, so the
|
||||
observable is the payload never being built — asserted through the builder
|
||||
rather than the scheduler, which needs a running loop.
|
||||
"""
|
||||
from unittest.mock import patch
|
||||
|
||||
import scribe.services.retrieval_telemetry as rt
|
||||
|
||||
with patch.object(rt, "_build_payload") as build:
|
||||
rt.record_retrieval(
|
||||
user_id=1, source="auto_inject", query="q", threshold=0.6,
|
||||
limit=3, project_id=None, is_task=None, results=[],
|
||||
searched=False,
|
||||
)
|
||||
assert not build.called, (
|
||||
"a search that never ran was recorded as a retrieval. It would "
|
||||
"read as a ranker decline — evidence about a threshold, from a "
|
||||
"call where no threshold was ever applied (#3765)"
|
||||
)
|
||||
|
||||
|
||||
def test_a_search_that_ran_and_found_nothing_still_writes_its_row():
|
||||
"""The half that stops the fix from being 'log less'.
|
||||
|
||||
A call that searched and came back empty is the ONLY evidence a threshold
|
||||
is too high (#3497). Dropping it too would trade one silent distortion for
|
||||
another, and this assertion is what makes the pair discriminate: a blanket
|
||||
`return` passes the test above and fails this one.
|
||||
"""
|
||||
from unittest.mock import patch
|
||||
|
||||
import scribe.services.retrieval_telemetry as rt
|
||||
|
||||
with patch.object(rt, "_build_payload") as build:
|
||||
rt.record_retrieval(
|
||||
user_id=1, source="auto_inject", query="q", threshold=0.6,
|
||||
limit=3, project_id=None, is_task=None, results=[],
|
||||
searched=True,
|
||||
)
|
||||
assert build.called, "a genuine zero-result call must still be recorded"
|
||||
|
||||
|
||||
def test_a_caller_that_never_asked_is_assumed_to_have_searched():
|
||||
"""`searched` defaults True, and the default is load-bearing.
|
||||
|
||||
A caller that passes no `report` cannot know, and the safe reading there is
|
||||
the old behaviour — log it. Only a REAL search can report False, because it
|
||||
stamps the key before anything can return. An absent key therefore means
|
||||
"nobody asked", never "it failed".
|
||||
"""
|
||||
from unittest.mock import patch
|
||||
|
||||
import scribe.services.retrieval_telemetry as rt
|
||||
|
||||
with patch.object(rt, "_build_payload") as build:
|
||||
rt.record_retrieval(
|
||||
user_id=1, source="mcp_search", query="q", threshold=0.45,
|
||||
limit=10, project_id=None, is_task=None, results=[],
|
||||
)
|
||||
assert build.called
|
||||
|
||||
@@ -0,0 +1,158 @@
|
||||
"""The SessionStart hook clears the rule ledger exactly when context dies (#3749).
|
||||
|
||||
WHY THIS EXISTS
|
||||
|
||||
The prior-art and tool-rule hooks record every rule id they have named in
|
||||
`<state>/<sid>.rules.ids` and hand it back as `exclude_rule_ids`, so a rule is
|
||||
surfaced once per session and then goes quiet. That is correct while the
|
||||
session still holds what it was told.
|
||||
|
||||
A compaction breaks that assumption in the worst available way: it summarizes
|
||||
the earlier injections out of context and does not touch the filesystem. The
|
||||
rule ends up absent from context AND still excluded — unreachable for the rest
|
||||
of the session. The compaction banner tells the model to re-pull its
|
||||
*always-on* rules, but a rule an arm surfaced is conditional and is not in that
|
||||
set, so it has no other way back. The rules most likely to be in that state are
|
||||
the ones that fire most often.
|
||||
|
||||
WHAT THIS PINS
|
||||
|
||||
Not "the ledger is cleared" — that would pass against a hook which deletes it
|
||||
on every source, and deleting on `resume` is its own defect: the context was
|
||||
genuinely restored there, so re-surfacing every rule is the mirror error.
|
||||
|
||||
What is pinned is the DISCRIMINATION. The whole source table is asserted in one
|
||||
statement, so a blanket delete (all False) and a no-op (all True) both fail,
|
||||
and neither can be made to pass by editing one case.
|
||||
|
||||
Runs the real shell against a temp TMPDIR, like the after-write hook's tests.
|
||||
Deliberately with no SCRIBE_URL/SCRIBE_TOKEN in the environment: clearing the
|
||||
ledger is local, keyless and networkless, and must still happen on an instance
|
||||
that is unreachable or unconfigured.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import os
|
||||
import shutil
|
||||
import subprocess
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
PLUGIN = Path(__file__).resolve().parents[1] / "plugin"
|
||||
HOOK = PLUGIN / "hooks" / "scribe_session_context.sh"
|
||||
|
||||
|
||||
def _env(tmp_path):
|
||||
"""Near-namesake of test_after_write_hook's `_env`, and deliberately not it:
|
||||
that one needs git and curl and SUPPLIES credentials, because the behaviour
|
||||
it tests is a network round-trip. This one must prove the opposite — that
|
||||
the clear happens with no credentials and no network at all — so sharing a
|
||||
helper would mean testing this case in an environment that cannot show it.
|
||||
"""
|
||||
for tool in ("jq", "bash"):
|
||||
if shutil.which(tool) is None:
|
||||
pytest.skip(f"hook runtime tool {tool!r} not installed")
|
||||
# No SCRIBE_URL / SCRIBE_TOKEN on purpose — see the module docstring.
|
||||
return {"PATH": os.environ["PATH"], "TMPDIR": str(tmp_path),
|
||||
"HOME": str(tmp_path)}
|
||||
|
||||
|
||||
def _ledger(tmp_path, sid: str, name: str = "rules.ids") -> Path:
|
||||
d = tmp_path / "scribe-priorart"
|
||||
d.mkdir(exist_ok=True)
|
||||
f = d / f"{sid}.{name}"
|
||||
f.write_text("156\n168\n")
|
||||
return f
|
||||
|
||||
|
||||
def _fire(source: str, sid: str, env) -> None:
|
||||
subprocess.run(
|
||||
["bash", str(HOOK)],
|
||||
input=json.dumps({"source": source, "session_id": sid}),
|
||||
capture_output=True, text=True, env=env, timeout=30,
|
||||
)
|
||||
|
||||
|
||||
def test_the_rules_ledger_survives_exactly_when_the_context_does(tmp_path):
|
||||
"""The whole source table, in one assertion, so it cannot be half-satisfied.
|
||||
|
||||
`startup` is listed even though it is a no-op against a session id that has
|
||||
never been seen: it is asserted here so that a future change which starts
|
||||
clearing indiscriminately fails on a case somebody would otherwise call
|
||||
harmless.
|
||||
"""
|
||||
env = _env(tmp_path)
|
||||
survived = {}
|
||||
for source in ("compact", "clear", "resume", "startup"):
|
||||
sid = f"sess-{source}"
|
||||
ledger = _ledger(tmp_path, sid)
|
||||
_fire(source, sid, env)
|
||||
survived[source] = ledger.exists()
|
||||
|
||||
assert survived == {
|
||||
"compact": False,
|
||||
"clear": False,
|
||||
"resume": True,
|
||||
"startup": True,
|
||||
}, (
|
||||
f"got {survived}. A rule surfaced before a compaction is summarized "
|
||||
f"out of context while its id stays on the exclusion ledger, so it "
|
||||
f"becomes unreachable for the rest of the session — that is what the "
|
||||
f"compact/clear cases prevent. The resume case is the other half: the "
|
||||
f"context came back intact there, and re-surfacing every rule after a "
|
||||
f"restore that lost nothing is the same defect from the other side. "
|
||||
f"All-False means something is deleting unconditionally; all-True "
|
||||
f"means the clear never runs."
|
||||
)
|
||||
|
||||
|
||||
def test_only_the_rule_ledger_is_cleared_and_the_note_ledgers_are_left(tmp_path):
|
||||
"""Scope, asserted rather than described.
|
||||
|
||||
The same directory holds `.ids`, `.sync.ids` and `.derive.ids` for the note
|
||||
arms. Whether a surfaced NOTE should come back after a compaction is a
|
||||
different question with a different answer, and it is not being answered.
|
||||
A `rm` glob over `<sid>.*` would pass every assertion in the test above
|
||||
while silently deciding it.
|
||||
"""
|
||||
env = _env(tmp_path)
|
||||
sid = "sess-scope"
|
||||
rules = _ledger(tmp_path, sid, "rules.ids")
|
||||
notes = _ledger(tmp_path, sid, "ids")
|
||||
sync = _ledger(tmp_path, sid, "sync.ids")
|
||||
derive = _ledger(tmp_path, sid, "derive.ids")
|
||||
|
||||
_fire("compact", sid, env)
|
||||
|
||||
assert not rules.exists(), "the rule ledger should have been cleared"
|
||||
assert notes.exists() and sync.exists() and derive.exists(), (
|
||||
"a note ledger was cleared too. The note arms were deliberately left "
|
||||
"out of #3749 — clearing them is a decision about a different surface, "
|
||||
"and a glob that takes them along makes it by accident."
|
||||
)
|
||||
|
||||
|
||||
def test_a_compact_without_a_session_id_is_survivable(tmp_path):
|
||||
"""Defensive, because this hook's contract is fail-open.
|
||||
|
||||
An event with no `session_id` names no ledger. The hook must not error, and
|
||||
must not fall back to a wildcard — clearing every session's ledger on the
|
||||
machine because this one event was malformed is the worst available
|
||||
reading of "best effort".
|
||||
"""
|
||||
env = _env(tmp_path)
|
||||
other = _ledger(tmp_path, "someone-elses-session")
|
||||
|
||||
proc = subprocess.run(
|
||||
["bash", str(HOOK)],
|
||||
input=json.dumps({"source": "compact"}),
|
||||
capture_output=True, text=True, env=env, timeout=30,
|
||||
)
|
||||
|
||||
assert proc.returncode == 0, proc.stderr
|
||||
assert other.exists(), (
|
||||
"an event with no session id cleared a ledger belonging to a different "
|
||||
"session"
|
||||
)
|
||||
@@ -1526,3 +1526,84 @@ def test_hook_keeps_the_rule_channel_apart_from_the_other_three():
|
||||
assert "(.rule_ids // [])[]?" in src # its own write-back
|
||||
# And it rides the same request as the rest, not a second round trip.
|
||||
assert "${rule_exclude_q}" in src
|
||||
|
||||
|
||||
# ── `best_available` describes the BAR, not this arm's own second filter ──────
|
||||
#
|
||||
# #3739 fixed the rule arms: a record the reader had already been shown was
|
||||
# being logged as something the ranker turned away, which made the near-miss
|
||||
# distribution report scores ABOVE the very threshold it is read against.
|
||||
#
|
||||
# The fix keyed on `suppressed_count`, and its NULL branch was justified by
|
||||
# "null means the caller passed its exclusions INTO the search, so the reported
|
||||
# score is already post-exclusion". That is true of auto_inject and reuse_slot.
|
||||
# It is NOT true here: this is the one note arm that filters twice. `exclude_ids`
|
||||
# takes `seen - pulled_seen` into the search, but the pulled-and-seen ids stay in
|
||||
# the query on purpose (the arm's query doubles as the resemblance test) and are
|
||||
# dropped afterwards in Python.
|
||||
#
|
||||
# Live proof, on the first read after that fix shipped: write_path's near-miss
|
||||
# max was 0.822 while the lowest score it ever RETURNED was 0.6857 — a
|
||||
# "rejection" that beat every acceptance.
|
||||
|
||||
|
||||
def _search_reporting(score, note):
|
||||
"""A stand-in search that fills `report` the way the real one does.
|
||||
|
||||
It ignores `exclude_ids`, which is exactly the condition being reproduced:
|
||||
a record that is in `seen` comes back from the search anyway. In production
|
||||
that happens because `pulled_seen` is deliberately left in the query; here
|
||||
it needs no ledger, and the arm's handling is the same either way.
|
||||
"""
|
||||
async def _search(uid, q, **kw):
|
||||
report = kw.get("report")
|
||||
if report is not None:
|
||||
report["best_available_score"] = score
|
||||
return [(score, note)]
|
||||
return _search
|
||||
|
||||
|
||||
async def _write_path_row(rec, **kwargs):
|
||||
from scribe.services import plugin_context as pc
|
||||
with patch.object(pc, "get_writepath_config", AsyncMock(return_value=_cfg())), \
|
||||
patch.object(pc.snippets_svc, "list_snippets", AsyncMock(return_value=([], 0))), \
|
||||
patch.object(pc, "record_retrieval", rec), \
|
||||
patch.object(pc, "owner_names_for", AsyncMock(return_value={})), \
|
||||
patch.object(pc, "semantic_search_notes",
|
||||
_search_reporting(0.9, fake_note(
|
||||
id=7, title="scored", user_id=1, note_type="snippet"))):
|
||||
await pc.build_write_path_hint(1, "src/x.py", code=REAL_CODE, **kwargs)
|
||||
return next(c for c in rec.call_args_list
|
||||
if c.kwargs["source"] == "write_path")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_record_this_arm_withheld_itself_is_not_a_near_miss():
|
||||
"""The defect: the row's count is POST this arm's filter and the score was
|
||||
captured PRE it, so a withheld record is indistinguishable from one the bar
|
||||
rejected — while scoring higher than anything the bar ever let through."""
|
||||
rec = MagicMock()
|
||||
row = await _write_path_row(rec, exclude_ids=[7])
|
||||
|
||||
assert row.kwargs["results"] == [], "the hit was withheld, so nothing shown"
|
||||
assert row.kwargs["best_available"] is None, (
|
||||
"a 0.9 record this arm withheld itself was reported as the best thing "
|
||||
"the THRESHOLD turned away. It would read as a bar set far too high "
|
||||
"when the bar never rejected it at all (#3739)"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_call_that_withheld_nothing_still_reports_what_the_bar_refused():
|
||||
"""The other half, and what stops the fix being 'never report it'.
|
||||
|
||||
Without this, setting `best_available=None` unconditionally passes the test
|
||||
above while deleting the measurement #3670 was built for.
|
||||
"""
|
||||
rec = MagicMock()
|
||||
row = await _write_path_row(rec)
|
||||
|
||||
assert row.kwargs["best_available"] == 0.9, (
|
||||
"nothing was withheld here, so the reported score describes the bar "
|
||||
"and must survive"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user