From 4107b177279e200f7a3cf8b69bf6d7ccf9bcc515 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 14 Aug 2026 21:49:48 -0400 Subject: [PATCH] =?UTF-8?q?fix(telemetry):=20usage=20readout=20grouped=20b?= =?UTF-8?q?y=20a=20rebuilt=20CASE=20=E2=80=94=20group=20by=20the=20label?= =?UTF-8?q?=20instead=20(#2663=20root=20cause)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .forgejo/workflows/ci.yml | 10 ++++++++ src/scribe/services/note_usage.py | 25 ++++++++++--------- tests/test_write_path_trigger.py | 40 ++++++++++++++++++++++++------- 3 files changed, 55 insertions(+), 20 deletions(-) diff --git a/.forgejo/workflows/ci.yml b/.forgejo/workflows/ci.yml index bcbde33..2acbc37 100644 --- a/.forgejo/workflows/ci.yml +++ b/.forgejo/workflows/ci.yml @@ -225,6 +225,16 @@ jobs: UV_PROJECT_ENVIRONMENT: /opt/venv run: uv sync --locked --extra dev + # The hook-EXECUTION tests (test_write_path_trigger's nudge pair) run the + # real bash hook, which exits silently without jq — and those tests skip + # rather than fail when it's absent, so without this step they would + # quietly never be verified anywhere (ci-python ships without jq; same + # install the Plugin hooks job does). + - name: Install jq for hook execution tests + run: | + apt-get update -qq + apt-get install -y -qq --no-install-recommends jq + - name: Run tests # Integration tests (real Postgres) run in the `integration` job below. run: /opt/venv/bin/python -m pytest tests/ -q -m "not integration" diff --git a/src/scribe/services/note_usage.py b/src/scribe/services/note_usage.py index 3cff648..6355c7d 100644 --- a/src/scribe/services/note_usage.py +++ b/src/scribe/services/note_usage.py @@ -186,6 +186,18 @@ async def usage_for_notes(note_ids: list[int]) -> dict[int, dict]: if not ids: return out + # Classified in SQL so the group count stays small: per note we get at most + # (surfaced-ranked, surfaced-ambient, pulled) rather than one row per + # distinct source. ONE labelled expression, grouped by its label — a second + # case() instance in GROUP BY renders with its own expanding-IN bind names + # under asyncpg, so the database sees two DIFFERENT expressions and rejects + # the query with a GroupingError. That rejection was swallowed, which is + # how every counter read zero in production while the writes were landing + # fine (#2663). + ambient = case( + (NoteUsageEvent.source.in_(AMBIENT_SOURCES), True), + else_=False, + ).label("ambient") try: async with async_session() as session: rows = ( @@ -195,22 +207,13 @@ async def usage_for_notes(note_ids: list[int]) -> dict[int, dict]: NoteUsageEvent.event, func.count().label("n"), func.max(NoteUsageEvent.created_at).label("last_at"), - # Classified in SQL so the group count stays small: per - # note we get at most (surfaced-ranked, surfaced-ambient, - # pulled) rather than one row per distinct source. - case( - (NoteUsageEvent.source.in_(AMBIENT_SOURCES), True), - else_=False, - ).label("ambient"), + ambient, ) .where(NoteUsageEvent.note_id.in_(ids)) .group_by( NoteUsageEvent.note_id, NoteUsageEvent.event, - case( - (NoteUsageEvent.source.in_(AMBIENT_SOURCES), True), - else_=False, - ), + ambient, ) ) ).all() diff --git a/tests/test_write_path_trigger.py b/tests/test_write_path_trigger.py index 8de1a11..7cf92e2 100644 --- a/tests/test_write_path_trigger.py +++ b/tests/test_write_path_trigger.py @@ -791,18 +791,39 @@ def test_plugin_version_bumped_with_the_hook(): assert version >= (0, 1, 18) +def _hook_runtime_env(): + """Env for tests that need the hook's tools to actually RUN. + + The silence-contract tests above deliberately restrict PATH — the hook must + exit quietly when its tools are missing, and asserting on empty output + doesn't care why it was empty. These tests assert on CONTENT, so the tools + must resolve wherever the image installed them; skip (don't fail) on an + image that lacks them, because that image cannot exercise this behaviour + at all. + """ + import os + import shutil + + for tool in ("git", "jq", "curl", "bash"): + if shutil.which(tool) is None: + pytest.skip(f"hook runtime tool {tool!r} not installed") + return {"PATH": os.environ["PATH"], + "SCRIBE_URL": "http://127.0.0.1:9", "SCRIBE_TOKEN": "t"} + + def test_hook_nudges_recording_when_copies_exist_but_nothing_is_recorded(tmp_path): """#2664: the local arm proves duplication; when Scribe has no record of it, the same context block must ask for create_snippet — the one moment the recording nudge is earned rather than noise. An unreachable server counts as "nothing recorded": the local finding needed no server, and the nudge fails open with it (here: a refused connection stands in for the instance).""" + env = _hook_runtime_env() repo = tmp_path / "repo" repo.mkdir() - subprocess.run(["git", "init", "-q"], cwd=repo, check=True) + subprocess.run(["git", "init", "-q"], cwd=repo, check=True, env=env) (repo / "a.py").write_text("def debounce(fn):\n return fn\n") # git grep searches the index, so the existing copy must be staged. - subprocess.run(["git", "add", "."], cwd=repo, check=True) + subprocess.run(["git", "add", "."], cwd=repo, check=True, env=env) out = subprocess.run( ["bash", str(HOOK)], input=json.dumps({ @@ -810,11 +831,13 @@ def test_hook_nudges_recording_when_copies_exist_but_nothing_is_recorded(tmp_pat "tool_input": {"file_path": str(repo / "b.py"), "content": "def debounce(fn):\n return fn\n"}, }), - capture_output=True, text=True, - env={"PATH": "/usr/bin:/bin", "SCRIBE_URL": "http://127.0.0.1:9", - "SCRIBE_TOKEN": "t"}, + capture_output=True, text=True, env=env, ) assert out.returncode == 0 + assert out.stdout.strip(), ( + "hook produced no output — the local arm should have found the " + "staged duplicate and nudged" + ) ctx = json.loads(out.stdout)["hookSpecificOutput"]["additionalContext"] assert "already defined" in ctx # the duplication finding assert "create_snippet" in ctx # the recording ask riding it @@ -823,9 +846,10 @@ def test_hook_nudges_recording_when_copies_exist_but_nothing_is_recorded(tmp_pat def test_hook_stays_quiet_about_recording_when_nothing_is_duplicated(tmp_path): """A brand-new helper with no other copies earns no nudge — a reflex that fires on every Write is one sessions learn to skip.""" + env = _hook_runtime_env() repo = tmp_path / "repo" repo.mkdir() - subprocess.run(["git", "init", "-q"], cwd=repo, check=True) + subprocess.run(["git", "init", "-q"], cwd=repo, check=True, env=env) out = subprocess.run( ["bash", str(HOOK)], input=json.dumps({ @@ -833,9 +857,7 @@ def test_hook_stays_quiet_about_recording_when_nothing_is_duplicated(tmp_path): "tool_input": {"file_path": str(repo / "b.py"), "content": "def debounce(fn):\n return fn\n"}, }), - capture_output=True, text=True, - env={"PATH": "/usr/bin:/bin", "SCRIBE_URL": "http://127.0.0.1:9", - "SCRIBE_TOKEN": "t"}, + capture_output=True, text=True, env=env, ) assert out.returncode == 0 assert "create_snippet" not in out.stdout