Commit Graph
5 Commits
Author SHA1 Message Date
bvandeusenandClaude Fable 5 7d48eb0b1b refactor(services): one periodic-task shape, one APScheduler job shape, one token hash, one summary rule — the services pass of the shape audit (#2830, milestone 296)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 7s
CI & Build / TypeScript typecheck (push) Successful in 12s
CI & Build / integration (push) Successful in 24s
CI & Build / Python tests (push) Successful in 55s
CI & Build / Build & push image (push) Successful in 24s
- background.start_periodic(interval, work, label=) replaces the three hand-rolled
  while-True/sleep/try loops in logging, auth and notifications.
- services/scheduler.ScheduledJob replaces the four private BackgroundScheduler
  copies in recurrence/version_pinning/trash/db_maintenance schedulers; public
  start_/stop_/reschedule_ surfaces unchanged.
- api_keys.hash_token is the one sha256 helper; auth.py used to inline it 5x.
- auth.is_registration_open reads via settings.get_admin_setting; notification
  prefs read via settings.get_setting; _fire_share_email uses _get_user_email.
- projects.get_project_summary / milestones.get_project_milestone_summary are
  now the one-id view of their batch siblings instead of a second copy of the
  queries; sharing.best_permission_by (was _deduplicate_by_permission) is the
  one rank-dedup, now also used by list_projects_for_user.
- backup: the row builders for every section both exporters carry are named
  functions, so a column added to one export cannot silently miss the other.
- iso() from models.base replaces the attr.isoformat()-if-attr-else-None idiom
  and db_maintenance._iso across services; backup keeps its explicit shape.
- trash.py hoists the sql_delete/timedelta imports it re-imported per function.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-08-21 12:34:28 -04:00
bvandeusenandClaude Fable 5 4107b17727 fix(telemetry): usage readout grouped by a rebuilt CASE — group by the label instead (#2663 root cause)
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / Python tests (push) Successful in 1m0s
CI & Build / Python lint (push) Successful in 4s
CI & Build / integration (push) Successful in 15s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / Build & push image (push) Successful in 39s
CI's new integration tests reproduced the outage on a clean database and
named the half: the writes land fine, and usage_for_notes fails on EVERY
call. The GROUP BY rebuilt the ambient case() expression, and asyncpg's
expanding IN-parameters give each instance its own bind names — so Postgres
sees a SELECT expression the GROUP BY doesn't cover and rejects the query
with a GroupingError, which the old code swallowed into zeros. One labelled
expression, grouped by its label. The deployed table has been accumulating
events all along; history appears as soon as this deploys.

Also: the two hook-execution tests now run with the real PATH and skip
when the hook's tools are absent (the restricted-PATH convention next door
is for silence contracts, where empty-for-the-wrong-reason still passes) —
and the unit lane installs jq so 'skip' never quietly becomes 'nowhere'.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-08-14 21:49:48 -04:00
bvandeusenandClaude Fable 5 77acee9239 fix(telemetry): usage counters get canaries, task references, and the missing integration tests (#2663)
The deployed instance ran with every usage counter at zero while surfacing
demonstrably fired. Every unit test was green because every unit test mocked
either _schedule or the session — the two functions that touch the database
ran against real Postgres nowhere. Both telemetry writers also held no
reference to their fire-and-forget tasks (the loop keeps only weak ones), and
swallowed every failure into logger.debug, so a total outage was
indistinguishable from an unused corpus.

- note_usage + retrieval_telemetry keep strong task references until done
- failures log at WARNING; note_usage additionally drops one AppLog error row
  per process per site, so the admin UI shows the outage without host access
- integration tests cover _insert_events -> usage_for_notes and the full
  record_pulled chain on a running loop, splitting the write and read halves
  so a failure names its side

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-08-14 21:45:32 -04:00
bvandeusen 2d1e26f38f feat(telemetry): ambient surfacings count, apart — enter_project and the skill sync emit
CI & Build / Python lint (push) Successful in 5s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 16s
CI & Build / TypeScript typecheck (push) Successful in 32s
CI & Build / Python tests (push) Successful in 47s
CI & Build / Build & push image (push) Successful in 25s
#2477, option (a) as decided, with the readout changed in the same commit.

## The two silent surfaces

enter_project returns open tasks + recent notes on every project entry —
probably the largest surfacing by volume — and emitted nothing, so the pulls
it caused floated unattributed and the surfaced:pulled ratio ran against a
denominator missing its biggest contributor. Now source "enter_project".

build_process_manifest installs every reachable Process as an auto-surfacing
skill on the operator's machine — its own docstring calls it the most
consequential passive surface Scribe has — and emitted nothing, so a Process
matched on every relevant turn and never opened was indistinguishable from
one never installed. Now source "process_skill_sync": the honest event is
"installed", which is a surfacing in effect since the description sits in
front of the model each session.

## The readout, same commit — the condition option (a) carried

Both surfaces are AMBIENT: top-N-by-recency and install-everything are not
ranked choices. Pooling them into surfaced_count would make a note's number
dominated by "recently updated in a project you opened", and dead-weight
detection would read that as popularity — the wrong number read confidently,
which is the corrupts-data tier the survey ranked above everything else.

So usage_for_notes splits: surfaced_count stays RANKED-ONLY (every existing
consumer's reading — "surfaced often, never pulled → dead weight" — keeps
meaning what it meant), and ambient_count is new. Classified in SQL via a
CASE on AMBIENT_SOURCES so the group count stays three rows per note, not one
per distinct source. Pulls stay pooled: "did anyone ever open this?" does not
depend on how it was found.

#1038 and #2085 read agent pulls and ranked surfacings; both are unaffected
by ambient volume, which is the point.

Refs #2477
2026-08-08 22:39:13 -04:00
bvandeusenandClaude Opus 5 2b85443dd1 feat(snippets): usage signal — was a surfaced record ever actually pulled?
CI & Build / Python lint (push) Successful in 2s
CI & Build / Python tests (push) Successful in 56s
CI & Build / integration (push) Successful in 31s
CI & Build / TypeScript typecheck (push) Successful in 34s
CI & Build / Build & push image (push) Successful in 44s
retrieval_logs answers "what did the ranker return, at what scores" — the
right substrate for tuning a threshold. It cannot answer the question the
snippet corpus actually needs: did anyone open this? A snippet nobody
opens is not neutral. It takes a slot in every future auto-inject menu and
crowds out something useful.

Adds note_usage_events (migration 0071): one row per note per event,
either 'surfaced' (we put its title in front of an agent) or 'pulled'
(someone opened it in full), tagged with which surface produced it.

Closes the gap #2082 recorded against this work. The write-path PLACE arm
carries no score, so it has no home in retrieval_logs — folding it in
would corrupt the score distribution that table exists to capture. The
result was that the arm firing on the STRONGEST claim ("there is already a
canonical helper in this exact file") was the one arm nobody could
measure. Both arms now emit usage events under distinct sources, so their
pull-through rates are finally comparable.

Deliberate departures from the task as written:

- Not in-session correlation. The original framing was "correlate
  result_ids against a later get_note in the same session." There is no
  session identity server-side — the MCP endpoint is stateless and the
  hooks send no session id — and adding one would mean threading an
  opaque client-supplied token through every read path. Two independent
  counters answer the question without it: surfaced 40×, pulled 0 is dead
  weight regardless of how those events distribute across sessions.

- Pulls record at the ENTRY POINTS (MCP tools, REST detail route), not in
  snippets_svc.get_snippet, which update and merge also reach. Counting
  those would inflate precisely the number meant to say "someone chose to
  look at this."

- get_note records for every note kind, not just snippets. The auto-inject
  menu surfaces tasks and processes too; scoping this to snippets would
  pin those at zero pulls forever and make them read as dead weight next
  to snippets that merely had a counter.

Surfaced in the Snippets list as an "N/M used" badge, warning-toned once a
record has been offered 3+ times and never opened, with the tooltip saying
what to do about it (usually: its "when to reach for it" doesn't say
when). No badge at all below one surfacing — "0/0" reads as a verdict when
it's an absence of evidence. Also returned from MCP list_snippets so the
agent can see dead weight without opening the UI.

Telemetry keeps the retrieval_telemetry contract throughout: writes are
fire-and-forget, reads degrade to zeroes, and no path can raise into the
surface it observes.

Refs #2085

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UaYUaouG9jjhATyuxCKrQs
2026-07-28 18:09:17 -04:00