fix(usage): one seam attaches the surfaced-vs-opened chip, and the Knowledge browse uses it (#4230)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 11s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 52s
CI & Build / Python tests (push) Failing after 1m2s
CI & Build / Build & push image (push) Skipped
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 11s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 52s
CI & Build / Python tests (push) Failing after 1m2s
CI & Build / Build & push image (push) Skipped
`usage_for_notes` is named for notes and works on every note row, yet the chip reached snippets and rules only. Notes had it nowhere. Lessons had it collected and shown nowhere a person could reach, because #4196 taught `/api/lessons` to attach it and `KnowledgeView` — the only lesson list in the UI — browses through `/api/knowledge`, so `listLessons` still has no consumer. The cause was not a missing line. SEVEN call sites carried their own copy of the same few lines: two REST lists, two REST details, two MCP lists, one MCP detail. Each read perfectly well alone, so "which doors attach usage?" had no answer anywhere in the code — the same asymmetry test_system_tagging_door_parity.py records for System tagging (#4249), where whichever door nobody exercised for a kind is the one that never grew the feature. `attach_usage(rows, key="id")` is now that answer, and all seven go through it. A detail payload is a one-row list, so the single-record doors share the seam rather than keeping a second shape beside it. Deliberately NO try/except: the fail-open already lives in `usage_for_notes`, which reports through `_report_failure("readout")` and returns the zero-filled map. Wrapping it again would swallow the REPORT as well as the error, and a silently-swallowed readout failure is exactly #2663 — every counter reading zero in production for weeks while the writes landed fine. `/api/knowledge` now attaches usage, which closes both holes at once: it is how notes, lessons and processes are all browsed. `KnowledgeView` renders the badge on the card footer, looking the advice up per row because the feed is mixed. The advice moves to utils/deadWeight.ts. Canon #3460 says each caller owns its own const, and that held while each caller showed ONE kind; a mixed feed would need five of its own and the next surface another five. The canon's actual invariant — advice is kind-specific and never baked into the badge — is kept: it is still a prop. The three existing callers now read the same table, so the sentence has one home rather than four. Recorded against #3460 so the next reader is not left re-litigating it. `_row_id` rejects bools explicitly: `int(True)` is 1, so a row carrying a flag under the key would be credited with note #1's counts, and a wrong chip is worse than no chip because it reads as a measurement. A row with no usable id is skipped rather than failing the page. Tests pin the PROPERTY, not one route: no door calls the aggregate directly (AST, so a comment naming it is not a false positive), and every door that shows usage reaches the seam. Plus the N+1 guard — one aggregate per page, asserted on await_count, because the per-row version reads more naturally and is invisible in review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
@@ -1,16 +1,7 @@
|
|||||||
<script setup lang="ts">
|
<script setup lang="ts">
|
||||||
import type { RuleHeader } from "@/api/rulebooks";
|
import type { RuleHeader } from "@/api/rulebooks";
|
||||||
import UsageBadge from "@/components/UsageBadge.vue";
|
import UsageBadge from "@/components/UsageBadge.vue";
|
||||||
|
import { DEAD_WEIGHT_ADVICE } from "@/utils/deadWeight";
|
||||||
/** The dead-weight nudge for a RULE — two remedies, not one, which is the
|
|
||||||
* whole reason this advice is per-kind. A snippet nobody opens should
|
|
||||||
* probably go. A rule nobody opens may be perfectly good and simply firing on
|
|
||||||
* the wrong thing, so "delete it" would be the wrong nudge half the time and
|
|
||||||
* the operator has to be the one who picks. */
|
|
||||||
const RULE_DEAD_WEIGHT =
|
|
||||||
"Kept arriving without being read. Either its trigger fires on the wrong " +
|
|
||||||
"work — reword “when to apply” so it says when — or it is not wanted here. " +
|
|
||||||
"Until one or the other, it takes a slot in every write it matches.";
|
|
||||||
|
|
||||||
defineProps<{ topicId: number; rules: RuleHeader[] }>();
|
defineProps<{ topicId: number; rules: RuleHeader[] }>();
|
||||||
const emit = defineEmits<{
|
const emit = defineEmits<{
|
||||||
@@ -54,7 +45,7 @@ const emit = defineEmits<{
|
|||||||
? 'Asserts a fact nobody has confirmed yet'
|
? 'Asserts a fact nobody has confirmed yet'
|
||||||
: `Check last passed ${r.last_verified}`"
|
: `Check last passed ${r.last_verified}`"
|
||||||
>{{ r.last_verified === "never" ? "unverified" : `checked ${r.last_verified}` }}</span>
|
>{{ r.last_verified === "never" ? "unverified" : `checked ${r.last_verified}` }}</span>
|
||||||
<UsageBadge :usage="r.usage" :dead-weight-advice="RULE_DEAD_WEIGHT" />
|
<UsageBadge :usage="r.usage" :dead-weight-advice="DEAD_WEIGHT_ADVICE.rule" />
|
||||||
</div>
|
</div>
|
||||||
<div class="statement">{{ r.statement }}</div>
|
<div class="statement">{{ r.statement }}</div>
|
||||||
<div v-if="r.when_to_apply || r.updated_at" class="meta">
|
<div v-if="r.when_to_apply || r.updated_at" class="meta">
|
||||||
|
|||||||
@@ -0,0 +1,68 @@
|
|||||||
|
/**
|
||||||
|
* What to suggest when a record looks like dead weight — one sentence per kind.
|
||||||
|
*
|
||||||
|
* `UsageBadge` deliberately takes the advice as a prop rather than deriving it,
|
||||||
|
* because the COUNTS read identically for every kind and the REMEDY does not: a
|
||||||
|
* snippet nobody opens should probably go, while a rule or a lesson in the same
|
||||||
|
* position more often has a trigger that fires on the wrong work. Telling an
|
||||||
|
* operator to delete one of those would be the wrong nudge about half the time.
|
||||||
|
*
|
||||||
|
* The table lives here because the copy was about to exist in five places.
|
||||||
|
* Three already had their own: `SnippetListView`, `RuleListPane`, and
|
||||||
|
* `LessonDetailView` with the sentence inline in its template. The unified
|
||||||
|
* Knowledge browse renders notes, tasks, processes, snippets and lessons in one
|
||||||
|
* mixed feed (#4230), so it needs all of them at once — and a per-view constant
|
||||||
|
* is how three surfaces end up giving three different answers to the same
|
||||||
|
* question.
|
||||||
|
*
|
||||||
|
* Every sentence names the COST of leaving it, not just the fact. "Never
|
||||||
|
* opened" is an observation; "takes a slot in every future menu" is why the
|
||||||
|
* reader should care, and it is the half that makes the chip actionable.
|
||||||
|
*/
|
||||||
|
|
||||||
|
/** The record kinds the Knowledge feed can show, plus the ones only their own
|
||||||
|
* views show. Keyed by `note_type`, with `rule` alongside — rules live in a
|
||||||
|
* separate table but answer the same question (see `RecordUsage`). */
|
||||||
|
export type DeadWeightKind =
|
||||||
|
| "note"
|
||||||
|
| "task"
|
||||||
|
| "process"
|
||||||
|
| "snippet"
|
||||||
|
| "lesson"
|
||||||
|
| "rule";
|
||||||
|
|
||||||
|
export const DEAD_WEIGHT_ADVICE: Record<DeadWeightKind, string> = {
|
||||||
|
snippet:
|
||||||
|
"Offered repeatedly without ever being opened — consider rewriting its " +
|
||||||
|
"“when to reach for it” so it says when, or deleting it. It takes a slot " +
|
||||||
|
"in every future auto-inject menu.",
|
||||||
|
rule:
|
||||||
|
"Kept arriving without being read. Either its trigger fires on the wrong " +
|
||||||
|
"work — reword “when to apply” so it says when — or it is not wanted here. " +
|
||||||
|
"Until one or the other, it takes a slot in every write it matches.",
|
||||||
|
lesson:
|
||||||
|
"Repeatedly offered and never opened usually means the trigger fires on " +
|
||||||
|
"the wrong situation — re-key `when_to_apply` rather than deleting the " +
|
||||||
|
"claim.",
|
||||||
|
note:
|
||||||
|
"Surfaced again and again and never opened. Usually the title is the " +
|
||||||
|
"problem: recall matches on it first, so a note titled for its author " +
|
||||||
|
"rather than for the situation keeps winning slots it cannot pay for.",
|
||||||
|
process:
|
||||||
|
"Offered without ever being run. Either the steps no longer match how " +
|
||||||
|
"the work is actually done, or it is being matched on the wrong trigger " +
|
||||||
|
"— check which before retiring it.",
|
||||||
|
task:
|
||||||
|
"Surfaced repeatedly and never opened. On a task this more often says " +
|
||||||
|
"the work has gone stale than that the record is wrong — decide whether " +
|
||||||
|
"it is still wanted before re-titling it.",
|
||||||
|
};
|
||||||
|
|
||||||
|
/** The advice for a kind, falling back to the note wording.
|
||||||
|
*
|
||||||
|
* The fallback is deliberate rather than an empty string: a kind added to the
|
||||||
|
* feed later should still get a usable sentence, and "the title is doing the
|
||||||
|
* matching" is the reading that holds for any record recall can choose. */
|
||||||
|
export function deadWeightAdvice(kind: string | null | undefined): string {
|
||||||
|
return DEAD_WEIGHT_ADVICE[kind as DeadWeightKind] ?? DEAD_WEIGHT_ADVICE.note;
|
||||||
|
}
|
||||||
@@ -3,7 +3,10 @@ import { ref, computed, watch, onMounted, onUnmounted, nextTick } from "vue";
|
|||||||
import { useRouter } from "vue-router";
|
import { useRouter } from "vue-router";
|
||||||
import { apiGet } from "@/api/client";
|
import { apiGet } from "@/api/client";
|
||||||
import type { TaskKind, TaskStatus, TaskPriority } from "@/types/note";
|
import type { TaskKind, TaskStatus, TaskPriority } from "@/types/note";
|
||||||
|
import type { RecordUsage } from "@/types/usage";
|
||||||
|
import { deadWeightAdvice } from "@/utils/deadWeight";
|
||||||
import KindBadge from "@/components/KindBadge.vue";
|
import KindBadge from "@/components/KindBadge.vue";
|
||||||
|
import UsageBadge from "@/components/UsageBadge.vue";
|
||||||
import NoteSweepPane from "@/components/NoteSweepPane.vue";
|
import NoteSweepPane from "@/components/NoteSweepPane.vue";
|
||||||
import StatusBadge from "@/components/StatusBadge.vue";
|
import StatusBadge from "@/components/StatusBadge.vue";
|
||||||
import PriorityBadge from "@/components/PriorityBadge.vue";
|
import PriorityBadge from "@/components/PriorityBadge.vue";
|
||||||
@@ -44,6 +47,10 @@ interface KnowledgeItem {
|
|||||||
project_id: number | null;
|
project_id: number | null;
|
||||||
created_at: string;
|
created_at: string;
|
||||||
updated_at: string;
|
updated_at: string;
|
||||||
|
/** Surfaced-vs-opened counts, zero-filled by the route for EVERY row, so
|
||||||
|
* "never surfaced" is a value here rather than a missing field (#4230).
|
||||||
|
* `UsageBadge` renders nothing at all below one surfacing. */
|
||||||
|
usage?: RecordUsage | null;
|
||||||
// Set only when another user owns this record — their suggestion, not one of
|
// Set only when another user owns this record — their suggestion, not one of
|
||||||
// yours. Absent means it's yours.
|
// yours. Absent means it's yours.
|
||||||
shared?: boolean;
|
shared?: boolean;
|
||||||
@@ -659,6 +666,16 @@ onUnmounted(() => {
|
|||||||
class="shared-tag"
|
class="shared-tag"
|
||||||
:title="`Shared by ${item.owner ?? 'another user'} — their record, not yours`"
|
:title="`Shared by ${item.owner ?? 'another user'} — their record, not yours`"
|
||||||
>by {{ item.owner ?? "another user" }}</span>
|
>by {{ item.owner ?? "another user" }}</span>
|
||||||
|
<!-- Surfaced-vs-opened (#4230). This is the only list in the UI
|
||||||
|
that browses notes and lessons, so it is the only place
|
||||||
|
those two kinds can show the counter at all. The advice is
|
||||||
|
looked up per row because this feed is mixed — the remedy
|
||||||
|
for a snippet nobody opens is not the remedy for a lesson. -->
|
||||||
|
<UsageBadge
|
||||||
|
:usage="item.usage"
|
||||||
|
:noun="item.note_type"
|
||||||
|
:dead-weight-advice="deadWeightAdvice(item.note_type)"
|
||||||
|
/>
|
||||||
<span class="k-card-date">{{ formatDate(item.updated_at) }}</span>
|
<span class="k-card-date">{{ formatDate(item.updated_at) }}</span>
|
||||||
</div>
|
</div>
|
||||||
</div>
|
</div>
|
||||||
|
|||||||
@@ -25,6 +25,7 @@ import { deleteLesson, getLesson, type Lesson } from "@/api/lessons";
|
|||||||
import ConfirmDialog from "@/components/ConfirmDialog.vue";
|
import ConfirmDialog from "@/components/ConfirmDialog.vue";
|
||||||
import TagPill from "@/components/TagPill.vue";
|
import TagPill from "@/components/TagPill.vue";
|
||||||
import UsageBadge from "@/components/UsageBadge.vue";
|
import UsageBadge from "@/components/UsageBadge.vue";
|
||||||
|
import { DEAD_WEIGHT_ADVICE } from "@/utils/deadWeight";
|
||||||
import { useToastStore } from "@/stores/toast";
|
import { useToastStore } from "@/stores/toast";
|
||||||
import { renderMarkdown } from "@/utils/markdown";
|
import { renderMarkdown } from "@/utils/markdown";
|
||||||
|
|
||||||
@@ -110,7 +111,7 @@ onMounted(load);
|
|||||||
<UsageBadge
|
<UsageBadge
|
||||||
:usage="lesson.usage"
|
:usage="lesson.usage"
|
||||||
noun="lesson"
|
noun="lesson"
|
||||||
dead-weight-advice="Repeatedly offered and never opened usually means the trigger fires on the wrong situation — re-key `when_to_apply` rather than deleting the claim."
|
:dead-weight-advice="DEAD_WEIGHT_ADVICE.lesson"
|
||||||
/>
|
/>
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
|
|||||||
@@ -10,6 +10,7 @@ import {
|
|||||||
} from "@/api/snippets";
|
} from "@/api/snippets";
|
||||||
import { useToastStore } from "@/stores/toast";
|
import { useToastStore } from "@/stores/toast";
|
||||||
import UsageBadge from "@/components/UsageBadge.vue";
|
import UsageBadge from "@/components/UsageBadge.vue";
|
||||||
|
import { DEAD_WEIGHT_ADVICE } from "@/utils/deadWeight";
|
||||||
|
|
||||||
const router = useRouter();
|
const router = useRouter();
|
||||||
const toast = useToastStore();
|
const toast = useToastStore();
|
||||||
@@ -237,14 +238,6 @@ function driftTitle(s: SnippetListItem): string {
|
|||||||
const what = reasons[v.status] ?? "";
|
const what = reasons[v.status] ?? "";
|
||||||
return v.detail ? `${when}: ${what}. ${v.detail}` : `${when}: ${what}.`;
|
return v.detail ? `${when}: ${what}. ${v.detail}` : `${when}: ${what}.`;
|
||||||
}
|
}
|
||||||
|
|
||||||
/** The dead-weight nudge for a SNIPPET, passed to the shared badge. Kept here
|
|
||||||
* rather than inside the component because the remedy is kind-specific — a
|
|
||||||
* rule in the same position gets different advice (milestone 333 step 5). */
|
|
||||||
const SNIPPET_DEAD_WEIGHT =
|
|
||||||
"Offered repeatedly without ever being opened — consider rewriting its " +
|
|
||||||
"“when to reach for it” so it says when, or deleting it. It takes a slot " +
|
|
||||||
"in every future auto-inject menu.";
|
|
||||||
</script>
|
</script>
|
||||||
|
|
||||||
<template>
|
<template>
|
||||||
@@ -431,7 +424,7 @@ const SNIPPET_DEAD_WEIGHT =
|
|||||||
<span v-if="driftBadge(s)" class="drift-tag" :title="driftTitle(s)">
|
<span v-if="driftBadge(s)" class="drift-tag" :title="driftTitle(s)">
|
||||||
{{ driftBadge(s) }}
|
{{ driftBadge(s) }}
|
||||||
</span>
|
</span>
|
||||||
<UsageBadge :usage="s.usage" :dead-weight-advice="SNIPPET_DEAD_WEIGHT" />
|
<UsageBadge :usage="s.usage" :dead-weight-advice="DEAD_WEIGHT_ADVICE.snippet" />
|
||||||
<span v-if="s.shared" class="shared-tag" :title="`Shared by ${s.owner ?? 'another user'} — a suggestion, not your own record`">
|
<span v-if="s.shared" class="shared-tag" :title="`Shared by ${s.owner ?? 'another user'} — a suggestion, not your own record`">
|
||||||
by {{ s.owner ?? "another user" }}
|
by {{ s.owner ?? "another user" }}
|
||||||
</span>
|
</span>
|
||||||
|
|||||||
@@ -17,7 +17,7 @@ from scribe.services import lessons as lessons_svc
|
|||||||
from scribe.services import systems as systems_svc
|
from scribe.services import systems as systems_svc
|
||||||
from scribe.services import trash as trash_svc
|
from scribe.services import trash as trash_svc
|
||||||
from scribe.mcp.tools import systems as systems_tools
|
from scribe.mcp.tools import systems as systems_tools
|
||||||
from scribe.services.note_usage import empty_usage, record_pulled, usage_for_notes
|
from scribe.services.note_usage import attach_usage, record_pulled
|
||||||
|
|
||||||
|
|
||||||
# The payload shape lives in the service (`lesson_to_dict`), shared with the
|
# The payload shape lives in the service (`lesson_to_dict`), shared with the
|
||||||
@@ -65,7 +65,7 @@ async def list_lessons(
|
|||||||
# per lesson (#4196). An agent listing lessons can see which of its own
|
# per lesson (#4196). An agent listing lessons can see which of its own
|
||||||
# triggers are firing and which are not, which is the reading that leads to
|
# triggers are firing and which are not, which is the reading that leads to
|
||||||
# `update_lesson` rather than to a second lesson about the same failure.
|
# `update_lesson` rather than to a second lesson about the same failure.
|
||||||
usage = await usage_for_notes([int(it["id"]) for it in labelled])
|
await attach_usage(labelled)
|
||||||
rows = [
|
rows = [
|
||||||
{
|
{
|
||||||
"id": it["id"], "title": it["title"], "tags": it.get("tags", []),
|
"id": it["id"], "title": it["title"], "tags": it.get("tags", []),
|
||||||
@@ -73,7 +73,7 @@ async def list_lessons(
|
|||||||
# Projected by `_note_to_item` straight off the `data` mirror —
|
# Projected by `_note_to_item` straight off the `data` mirror —
|
||||||
# absent when the row carries none, rather than an empty string.
|
# absent when the row carries none, rather than an empty string.
|
||||||
"when_to_apply": it.get("when_to_apply", ""),
|
"when_to_apply": it.get("when_to_apply", ""),
|
||||||
"usage": usage.get(int(it["id"]), empty_usage()),
|
"usage": it["usage"],
|
||||||
**({"shared": True, "owner": it.get("owner")} if it.get("shared") else {}),
|
**({"shared": True, "owner": it.get("owner")} if it.get("shared") else {}),
|
||||||
}
|
}
|
||||||
for it in labelled
|
for it in labelled
|
||||||
@@ -212,9 +212,7 @@ async def get_lesson(lesson_id: int, project_id: int = 0) -> dict:
|
|||||||
# Read BEFORE the pull is recorded, so the number an agent is shown is the
|
# Read BEFORE the pull is recorded, so the number an agent is shown is the
|
||||||
# one that was true when it asked — otherwise every first read of a lesson
|
# one that was true when it asked — otherwise every first read of a lesson
|
||||||
# reports a pull that is its own.
|
# reports a pull that is its own.
|
||||||
out["usage"] = (await usage_for_notes([int(note.id)])).get(
|
await attach_usage([out])
|
||||||
int(note.id), empty_usage()
|
|
||||||
)
|
|
||||||
record_pulled(
|
record_pulled(
|
||||||
user_id=uid, note_id=int(note.id),
|
user_id=uid, note_id=int(note.id),
|
||||||
source="mcp_get_lesson", project_id=project_id,
|
source="mcp_get_lesson", project_id=project_id,
|
||||||
|
|||||||
@@ -15,7 +15,7 @@ from scribe.mcp.tools import systems as systems_tools
|
|||||||
from scribe.services import access as access_svc
|
from scribe.services import access as access_svc
|
||||||
from scribe.services import dedup as dedup_svc
|
from scribe.services import dedup as dedup_svc
|
||||||
from scribe.services import snippets as snippets_svc
|
from scribe.services import snippets as snippets_svc
|
||||||
from scribe.services.note_usage import empty_usage, record_pulled, usage_for_notes
|
from scribe.services.note_usage import attach_usage, record_pulled
|
||||||
from scribe.services import systems as systems_svc
|
from scribe.services import systems as systems_svc
|
||||||
|
|
||||||
|
|
||||||
@@ -90,9 +90,7 @@ async def list_snippets(
|
|||||||
repo=repo, path=path, symbol=symbol, verification=verification,
|
repo=repo, path=path, symbol=symbol, verification=verification,
|
||||||
)
|
)
|
||||||
labeled = await access_svc.label_shared_items(uid, items)
|
labeled = await access_svc.label_shared_items(uid, items)
|
||||||
usage = await usage_for_notes([int(it["id"]) for it in labeled])
|
await attach_usage(labeled)
|
||||||
for it in labeled:
|
|
||||||
it["usage"] = usage.get(int(it["id"]), empty_usage())
|
|
||||||
return {"snippets": labeled, "total": total}
|
return {"snippets": labeled, "total": total}
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ from scribe.auth import get_current_user_id, login_required
|
|||||||
from scribe.routes.utils import parse_pagination
|
from scribe.routes.utils import parse_pagination
|
||||||
from scribe.services.access import label_shared_items
|
from scribe.services.access import label_shared_items
|
||||||
from scribe.services.knowledge import FACET_TYPES
|
from scribe.services.knowledge import FACET_TYPES
|
||||||
|
from scribe.services.note_usage import attach_usage
|
||||||
|
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
@@ -62,10 +63,23 @@ async def list_knowledge():
|
|||||||
offset=offset,
|
offset=offset,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
items = await label_shared_items(uid, items)
|
||||||
|
# The surfaced-vs-opened counts, on the list a person actually browses
|
||||||
|
# (#4230). `usage_for_notes` always worked on every note row, but only the
|
||||||
|
# snippet and rule lists ever attached it — and this is the ONLY lesson and
|
||||||
|
# note list in the UI, so those two kinds had the counter collected and
|
||||||
|
# shown nowhere. Attaching here rather than teaching `/api/lessons` a
|
||||||
|
# second time is what closes both holes at once: `/knowledge` is how notes,
|
||||||
|
# lessons and processes are all browsed.
|
||||||
|
#
|
||||||
|
# Mixed kinds is not a problem for this: usage keys on the note row, which
|
||||||
|
# every facet of this feed is.
|
||||||
|
await attach_usage(items)
|
||||||
|
|
||||||
return jsonify({
|
return jsonify({
|
||||||
# Mark rows another user owns: this feed can be mixed-ownership, and an
|
# Mark rows another user owns: this feed can be mixed-ownership, and an
|
||||||
# unmarked card reads as one the viewer wrote.
|
# unmarked card reads as one the viewer wrote.
|
||||||
"items": await label_shared_items(uid, items),
|
"items": items,
|
||||||
"total": total,
|
"total": total,
|
||||||
"page": page,
|
"page": page,
|
||||||
"per_page": limit,
|
"per_page": limit,
|
||||||
|
|||||||
@@ -36,7 +36,7 @@ from scribe.services.access import (
|
|||||||
describe_provenance,
|
describe_provenance,
|
||||||
label_shared_items,
|
label_shared_items,
|
||||||
)
|
)
|
||||||
from scribe.services.note_usage import empty_usage, record_pulled, usage_for_notes
|
from scribe.services.note_usage import attach_usage, record_pulled
|
||||||
|
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
@@ -83,9 +83,7 @@ async def list_lessons_route():
|
|||||||
# surfaced AND repeatedly opened, and the far commoner reading of the same
|
# surfaced AND repeatedly opened, and the far commoner reading of the same
|
||||||
# row is that the trigger fires on the wrong situation, which `update_lesson`
|
# row is that the trigger fires on the wrong situation, which `update_lesson`
|
||||||
# exists to fix.
|
# exists to fix.
|
||||||
usage = await usage_for_notes([int(it["id"]) for it in items])
|
await attach_usage(items)
|
||||||
for it in items:
|
|
||||||
it["usage"] = usage.get(int(it["id"]), empty_usage())
|
|
||||||
return jsonify({"lessons": items, "total": total})
|
return jsonify({"lessons": items, "total": total})
|
||||||
|
|
||||||
|
|
||||||
@@ -194,9 +192,7 @@ async def get_lesson_route(lesson_id: int):
|
|||||||
uid, out["learned_from"]
|
uid, out["learned_from"]
|
||||||
)
|
)
|
||||||
out.update(await describe_provenance(uid, note))
|
out.update(await describe_provenance(uid, note))
|
||||||
out["usage"] = (await usage_for_notes([lesson_id])).get(
|
await attach_usage([out])
|
||||||
lesson_id, empty_usage()
|
|
||||||
)
|
|
||||||
# Opening the detail view IS a pull — the operator chose to look. Tagged
|
# Opening the detail view IS a pull — the operator chose to look. Tagged
|
||||||
# apart from the MCP sources so "an agent was handed it" and "a human read
|
# apart from the MCP sources so "an agent was handed it" and "a human read
|
||||||
# it" stay distinguishable; they mean different things for pruning (#2085).
|
# it" stay distinguishable; they mean different things for pruning (#2085).
|
||||||
|
|||||||
@@ -19,7 +19,7 @@ from scribe.routes.utils import not_found, parse_pagination
|
|||||||
from scribe.services import dedup as dedup_svc
|
from scribe.services import dedup as dedup_svc
|
||||||
from scribe.services import snippets as snippets_svc
|
from scribe.services import snippets as snippets_svc
|
||||||
from scribe.services import systems as systems_svc
|
from scribe.services import systems as systems_svc
|
||||||
from scribe.services.note_usage import empty_usage, record_pulled, usage_for_notes
|
from scribe.services.note_usage import attach_usage, record_pulled
|
||||||
from scribe.services.access import (
|
from scribe.services.access import (
|
||||||
can_write_note,
|
can_write_note,
|
||||||
describe_provenance,
|
describe_provenance,
|
||||||
@@ -75,9 +75,7 @@ async def list_snippets_route():
|
|||||||
# One aggregate for the whole page — a per-row lookup here would be N+1 by
|
# One aggregate for the whole page — a per-row lookup here would be N+1 by
|
||||||
# construction. Every row gets the key, zero-filled, so the UI renders
|
# construction. Every row gets the key, zero-filled, so the UI renders
|
||||||
# "never pulled" rather than having to treat a missing field as a state.
|
# "never pulled" rather than having to treat a missing field as a state.
|
||||||
usage = await usage_for_notes([int(it["id"]) for it in items])
|
await attach_usage(items)
|
||||||
for it in items:
|
|
||||||
it["usage"] = usage.get(int(it["id"]), empty_usage())
|
|
||||||
return jsonify({"snippets": items, "total": total})
|
return jsonify({"snippets": items, "total": total})
|
||||||
|
|
||||||
|
|
||||||
@@ -168,9 +166,7 @@ async def get_snippet_route(snippet_id: int):
|
|||||||
for s in await systems_svc.list_record_systems(note.user_id, snippet_id)
|
for s in await systems_svc.list_record_systems(note.user_id, snippet_id)
|
||||||
]
|
]
|
||||||
data.update(await describe_provenance(uid, note))
|
data.update(await describe_provenance(uid, note))
|
||||||
data["usage"] = (await usage_for_notes([snippet_id])).get(
|
await attach_usage([data])
|
||||||
snippet_id, empty_usage()
|
|
||||||
)
|
|
||||||
# Opening the detail view IS a pull — the operator chose to look. Tagged
|
# Opening the detail view IS a pull — the operator chose to look. Tagged
|
||||||
# apart from the MCP sources so "the agent reused it" and "a human read it"
|
# apart from the MCP sources so "the agent reused it" and "a human read it"
|
||||||
# stay distinguishable; they mean different things for pruning (#2085).
|
# stay distinguishable; they mean different things for pruning (#2085).
|
||||||
|
|||||||
@@ -30,6 +30,7 @@ from __future__ import annotations
|
|||||||
|
|
||||||
import asyncio
|
import asyncio
|
||||||
import logging
|
import logging
|
||||||
|
from collections.abc import Sequence
|
||||||
|
|
||||||
from sqlalchemy import case, func, select
|
from sqlalchemy import case, func, select
|
||||||
|
|
||||||
@@ -271,3 +272,64 @@ async def usage_for_notes(note_ids: list[int]) -> dict[int, dict]:
|
|||||||
if latest and (slot["last_pulled_at"] or "") < latest:
|
if latest and (slot["last_pulled_at"] or "") < latest:
|
||||||
slot["last_pulled_at"] = latest
|
slot["last_pulled_at"] = latest
|
||||||
return out
|
return out
|
||||||
|
|
||||||
|
|
||||||
|
def _row_id(row: dict, key: str) -> int | None:
|
||||||
|
"""The note id on a payload row, or None when there is not one to read.
|
||||||
|
|
||||||
|
Skipping is deliberate: an id this cannot parse is not a reason to fail a
|
||||||
|
whole list, and GUESSING one would credit another record's counts to this
|
||||||
|
row — a wrong chip is worse than no chip, because it reads as a
|
||||||
|
measurement. `bool` is excluded explicitly because `int(True)` is 1, which
|
||||||
|
would quietly attach note #1's usage to a row carrying a flag.
|
||||||
|
"""
|
||||||
|
raw = row.get(key)
|
||||||
|
if raw is None or isinstance(raw, bool):
|
||||||
|
return None
|
||||||
|
try:
|
||||||
|
return int(raw)
|
||||||
|
except (TypeError, ValueError):
|
||||||
|
return None
|
||||||
|
|
||||||
|
|
||||||
|
async def attach_usage(rows: Sequence[dict], *, key: str = "id") -> None:
|
||||||
|
"""Add `usage` to every row of a payload a door is about to return (#4230).
|
||||||
|
|
||||||
|
The one seam both doors and every record kind share. Before this, four call
|
||||||
|
sites carried their own copy of the same lines — two list routes and two
|
||||||
|
detail routes — and `/api/knowledge`, which is the list a person ACTUALLY
|
||||||
|
browses notes and lessons in, was about to become a fifth. That is how the
|
||||||
|
chip came to reach two record kinds out of four while a service named
|
||||||
|
`usage_for_notes` worked on all of them: each door read fine on its own,
|
||||||
|
and nobody was comparing them.
|
||||||
|
|
||||||
|
ONE AGGREGATE FOR THE WHOLE PAGE. `usage_for_notes` is a single GROUP BY
|
||||||
|
over the id set; calling it per row would be N+1 by construction, which is
|
||||||
|
the one shape a list route must not have.
|
||||||
|
|
||||||
|
EVERY ROW GETS THE KEY, zero-filled, so a record predating the table reads
|
||||||
|
as "never surfaced, never pulled" rather than making the UI treat a missing
|
||||||
|
field as a state. `UsageBadge` then renders nothing at all below one
|
||||||
|
surfacing, because "0/0" would look like a verdict where there is only an
|
||||||
|
absence of evidence.
|
||||||
|
|
||||||
|
NO try/except HERE, deliberately — it is not an oversight. The fail-open
|
||||||
|
already lives one layer down: `usage_for_notes` catches its own failure,
|
||||||
|
reports it through `_report_failure("readout")` and returns the zero-filled
|
||||||
|
map, so a broken readout degrades without breaking the list it decorates.
|
||||||
|
Wrapping it again would swallow the REPORT along with the error, and a
|
||||||
|
silently-swallowed readout failure is exactly #2663 — every counter reading
|
||||||
|
zero in production for weeks while the writes landed fine.
|
||||||
|
|
||||||
|
Mutates in place and returns None, matching how the call sites already used
|
||||||
|
it: these rows are the payload, not a copy of it.
|
||||||
|
|
||||||
|
A detail payload is just a one-row list — `await attach_usage([data])` —
|
||||||
|
so the single-record doors share this seam rather than keeping a second
|
||||||
|
shape that could drift from it.
|
||||||
|
"""
|
||||||
|
pairs = [(row, _row_id(row, key)) for row in rows]
|
||||||
|
usage = await usage_for_notes([nid for _, nid in pairs if nid is not None])
|
||||||
|
for row, nid in pairs:
|
||||||
|
if nid is not None:
|
||||||
|
row["usage"] = usage.get(nid, empty_usage())
|
||||||
|
|||||||
@@ -0,0 +1,205 @@
|
|||||||
|
"""One seam attaches `usage`, and every door that shows it uses that seam (#4230).
|
||||||
|
|
||||||
|
WHAT WENT WRONG. `usage_for_notes` is named for notes and works on every note
|
||||||
|
row. Yet the surfaced-vs-opened chip reached snippets and rules only: notes had
|
||||||
|
it nowhere, and lessons had it collected but shown nowhere a person could
|
||||||
|
reach, because the only lesson LIST in the UI is the Knowledge browse and that
|
||||||
|
route never attached it.
|
||||||
|
|
||||||
|
The cause was not any one missing line. Seven call sites carried their own copy
|
||||||
|
of the same few lines — two REST lists, two REST details, two MCP lists, one
|
||||||
|
MCP detail — and each read perfectly well on its own. Nobody was comparing
|
||||||
|
them, so "which doors attach usage?" had no answer anywhere in the code. That
|
||||||
|
is the same failure `test_system_tagging_door_parity.py` records for System
|
||||||
|
tagging (#4249): whichever door nobody exercised for a kind is the one that
|
||||||
|
never grew the feature, and a human reviewer does not reliably catch it because
|
||||||
|
each door is only ever read alone.
|
||||||
|
|
||||||
|
So this file asserts the PROPERTY, not the behaviour of one route: the attach
|
||||||
|
logic exists once, and no door re-implements it. A kind added next month either
|
||||||
|
goes through the seam or fails here.
|
||||||
|
"""
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import ast
|
||||||
|
import pathlib
|
||||||
|
from unittest.mock import AsyncMock, patch
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
from scribe.services.note_usage import attach_usage, empty_usage
|
||||||
|
|
||||||
|
ROOT = pathlib.Path(__file__).resolve().parents[1] / "src" / "scribe"
|
||||||
|
|
||||||
|
# The aggregate the seam is built around. Calling it from a door is the shape
|
||||||
|
# this file exists to prevent — not because the call is wrong, but because
|
||||||
|
# seven of them drift.
|
||||||
|
AGGREGATE = "usage_for_notes"
|
||||||
|
|
||||||
|
|
||||||
|
def _counts(surfaced: int = 5, pulled: int = 0) -> dict:
|
||||||
|
u = empty_usage()
|
||||||
|
u["surfaced_count"] = surfaced
|
||||||
|
u["pull_count"] = pulled
|
||||||
|
return u
|
||||||
|
|
||||||
|
|
||||||
|
def _aggregate_returns(mapping: dict[int, dict]) -> AsyncMock:
|
||||||
|
return patch(
|
||||||
|
"scribe.services.note_usage.usage_for_notes",
|
||||||
|
AsyncMock(return_value=mapping),
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
# ── the seam itself ───────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
|
||||||
|
async def test_every_row_gets_the_key_even_with_no_events() -> None:
|
||||||
|
"""Zero-filled, never absent. The UI must not have to tell "no events"
|
||||||
|
from "no field" — and `UsageBadge` renders nothing below one surfacing, so
|
||||||
|
an un-surfaced record is quiet without the caller doing anything."""
|
||||||
|
rows = [{"id": 1}, {"id": 2}]
|
||||||
|
with _aggregate_returns({1: _counts(surfaced=3)}):
|
||||||
|
await attach_usage(rows)
|
||||||
|
assert rows[0]["usage"]["surfaced_count"] == 3
|
||||||
|
assert rows[1]["usage"] == empty_usage()
|
||||||
|
|
||||||
|
|
||||||
|
async def test_one_aggregate_for_the_whole_page() -> None:
|
||||||
|
"""The N+1 guard. A per-row lookup here would be N+1 by construction, which
|
||||||
|
is the one shape a list route must not have — and it is invisible in
|
||||||
|
review, because the per-row version reads more naturally."""
|
||||||
|
rows = [{"id": n} for n in range(25)]
|
||||||
|
mock = AsyncMock(return_value={})
|
||||||
|
with patch("scribe.services.note_usage.usage_for_notes", mock):
|
||||||
|
await attach_usage(rows)
|
||||||
|
assert mock.await_count == 1, "usage must be read once per page, not per row"
|
||||||
|
assert sorted(mock.await_args.args[0]) == list(range(25))
|
||||||
|
|
||||||
|
|
||||||
|
async def test_a_detail_payload_is_just_a_one_row_list() -> None:
|
||||||
|
"""The single-record doors share the seam rather than keeping a second
|
||||||
|
shape beside it. Two shapes for one job is how the seven copies started."""
|
||||||
|
data = {"id": 7, "title": "x"}
|
||||||
|
with _aggregate_returns({7: _counts(surfaced=9, pulled=2)}):
|
||||||
|
await attach_usage([data])
|
||||||
|
assert data["usage"]["pull_count"] == 2
|
||||||
|
|
||||||
|
|
||||||
|
async def test_a_row_with_no_id_is_skipped_rather_than_failing_the_list() -> None:
|
||||||
|
"""An unusable id is not a reason to 500 a page of otherwise fine rows."""
|
||||||
|
rows = [{"id": 1}, {"title": "no id here"}]
|
||||||
|
with _aggregate_returns({1: _counts()}):
|
||||||
|
await attach_usage(rows)
|
||||||
|
assert "usage" in rows[0]
|
||||||
|
assert "usage" not in rows[1]
|
||||||
|
|
||||||
|
|
||||||
|
async def test_a_boolean_is_not_an_id() -> None:
|
||||||
|
"""`int(True)` is 1, so a row carrying a flag under the key would silently
|
||||||
|
be credited with note #1's counts. A wrong chip is worse than no chip: it
|
||||||
|
reads as a measurement."""
|
||||||
|
rows = [{"id": True}]
|
||||||
|
with _aggregate_returns({1: _counts(surfaced=40)}):
|
||||||
|
await attach_usage(rows)
|
||||||
|
assert "usage" not in rows[0]
|
||||||
|
|
||||||
|
|
||||||
|
async def test_a_string_id_still_resolves() -> None:
|
||||||
|
"""Payload rows come from several serialisers; one of them handing back a
|
||||||
|
stringified id should not silently drop the chip."""
|
||||||
|
rows = [{"id": "12"}]
|
||||||
|
with _aggregate_returns({12: _counts(surfaced=4)}):
|
||||||
|
await attach_usage(rows)
|
||||||
|
assert rows[0]["usage"]["surfaced_count"] == 4
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("key", ["note_id", "record_id"])
|
||||||
|
async def test_the_key_can_be_named(key: str) -> None:
|
||||||
|
rows = [{key: 3}]
|
||||||
|
with _aggregate_returns({3: _counts()}):
|
||||||
|
await attach_usage(rows, key=key)
|
||||||
|
assert "usage" in rows[0]
|
||||||
|
|
||||||
|
|
||||||
|
async def test_an_empty_page_asks_nothing_and_breaks_nothing() -> None:
|
||||||
|
mock = AsyncMock(return_value={})
|
||||||
|
with patch("scribe.services.note_usage.usage_for_notes", mock):
|
||||||
|
await attach_usage([])
|
||||||
|
assert mock.await_args.args[0] == []
|
||||||
|
|
||||||
|
|
||||||
|
# ── the property: one seam, and every door uses it ────────────────────────
|
||||||
|
|
||||||
|
|
||||||
|
def _calls(tree: ast.Module) -> set[str]:
|
||||||
|
out = set()
|
||||||
|
for node in ast.walk(tree):
|
||||||
|
if isinstance(node, ast.Call):
|
||||||
|
fn = node.func
|
||||||
|
name = fn.attr if isinstance(fn, ast.Attribute) else getattr(fn, "id", None)
|
||||||
|
if name:
|
||||||
|
out.add(name)
|
||||||
|
return out
|
||||||
|
|
||||||
|
|
||||||
|
def _door_modules() -> list[pathlib.Path]:
|
||||||
|
return sorted(
|
||||||
|
[*(ROOT / "routes").glob("*.py"), *(ROOT / "mcp" / "tools").glob("*.py")]
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_no_door_calls_the_aggregate_directly() -> None:
|
||||||
|
"""THE GUARD. Seven doors each called `usage_for_notes` and zero-filled by
|
||||||
|
hand; the eighth would have been `/api/knowledge`, and the chip would have
|
||||||
|
kept reaching some kinds and not others.
|
||||||
|
|
||||||
|
Keyed on the CALL, not on the text, so a module that merely names the
|
||||||
|
function in a comment explaining the seam is not a false positive — and a
|
||||||
|
hand-kept skip list, which would itself go stale, is not needed (rule 167).
|
||||||
|
"""
|
||||||
|
offenders = []
|
||||||
|
for path in _door_modules():
|
||||||
|
if AGGREGATE in _calls(ast.parse(path.read_text())):
|
||||||
|
offenders.append(str(path.relative_to(ROOT.parent.parent)))
|
||||||
|
assert not offenders, (
|
||||||
|
f"these doors call {AGGREGATE}() themselves instead of attach_usage(); "
|
||||||
|
f"that is how the chip came to reach two record kinds out of four: "
|
||||||
|
f"{offenders}"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
# (module, the functions that return note-bearing payloads)
|
||||||
|
#
|
||||||
|
# Not a list of everything that COULD attach usage — a list of the doors that
|
||||||
|
# demonstrably show it today. A door dropping its call silently is the exact
|
||||||
|
# regression this pins.
|
||||||
|
DOORS = [
|
||||||
|
("routes/lessons.py", "list_lessons_route or get_lesson_route"),
|
||||||
|
("routes/snippets.py", "list/get snippet routes"),
|
||||||
|
("routes/knowledge.py", "list_knowledge — the only note & lesson list in the UI"),
|
||||||
|
("mcp/tools/lessons.py", "list_lessons / get_lesson"),
|
||||||
|
("mcp/tools/snippets.py", "list_snippets"),
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(("module", "why"), DOORS)
|
||||||
|
def test_every_door_that_shows_usage_goes_through_the_seam(module: str, why: str) -> None:
|
||||||
|
assert "attach_usage" in _calls(ast.parse((ROOT / module).read_text())), (
|
||||||
|
f"{module} no longer attaches usage ({why}). If that is deliberate, "
|
||||||
|
f"remove it from DOORS and say why; a door that silently stops "
|
||||||
|
f"attaching looks exactly like a corpus nobody uses."
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_the_knowledge_browse_is_covered_because_it_is_the_only_note_list() -> None:
|
||||||
|
"""Pinned on its own, with the reason, because it is the non-obvious one.
|
||||||
|
|
||||||
|
`/api/lessons` already attached usage and it did not help: no view calls
|
||||||
|
it. `KnowledgeView` is the only list in the UI that renders notes and
|
||||||
|
lessons, so `/api/knowledge` is the only route through which those two
|
||||||
|
kinds can show the counter at all. Deleting this line would restore the
|
||||||
|
original bug while every other test here still passed.
|
||||||
|
"""
|
||||||
|
assert any(m == "routes/knowledge.py" for m, _ in DOORS)
|
||||||
|
assert "attach_usage" in _calls(ast.parse((ROOT / "routes" / "knowledge.py").read_text()))
|
||||||
Reference in New Issue
Block a user