From 97867d47ff585ece6e1c09e3f79a83927e652784 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 19 Sep 2026 23:32:54 -0400 Subject: [PATCH] fix(telemetry): the reading project survives a backup (#4196) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run 7068 failed two tests on a4aae97, and the second was a real bug the guard caught before it shipped. THE COLUMN GUARD (#3182) was right. `note_usage_events` excludes only `id` from backup, so telemetry IS exported — and a new column that the serialiser never learned about would have restored as null on every row. That is the exact failure #3182 was built from, where nine columns vanished from `notes` the same way: added to the model and the migration, which fail loudly, and never to the serialiser, which fails silently. So `project_id` is exported, not excluded, via the one `_usage_event_rows` builder both exporters share. The importer re-maps it through `project_id_map` and DEGRADES rather than skipping. `code_shape_events` drops a row whose project will not map, because a shape event without its project says nothing; a usage event is not like that. The project is optional by design and null already means "not reported", so an unmappable one restores as unreported. Dropping it would lose a real pull and deflate the pull-through this table exists to report. BACKUP_VERSION deliberately unchanged: the suite states that column additions do not bump it, only new sections do, and a third test asserts the current number. The second failure was mine and smaller — test_record_pulled_writes_a_ single_row compares exact dicts, so it now expects the null project. I had claimed these assertions were key-based; they are not, and I had read a summary line rather than the assertion. Added an import-side guard while here. The column family checks the EXPORT side only, so a column can be exported faithfully and dropped on the way back in — a restore that reports success and has quietly lost a dimension. No general guard for that direction exists yet; this covers the column added here and names the gap for whoever builds the general one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/services/backup.py | 8 +++++++- tests/test_note_usage.py | 3 ++- tests/test_services_backup.py | 36 +++++++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/src/scribe/services/backup.py b/src/scribe/services/backup.py index cf7573e..735b559 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -333,7 +333,7 @@ def _usage_event_rows(rows) -> list[dict]: return [ { "user_id": r.user_id, "note_id": r.note_id, "event": r.event, - "source": r.source, + "source": r.source, "project_id": r.project_id, "created_at": r.created_at.isoformat() if r.created_at else None, } for r in rows @@ -1548,6 +1548,12 @@ async def _restore_v2(data: dict) -> dict: note_id=mapped_nid, event=ev.get("event", ""), source=ev.get("source", ""), + # Degrades to None rather than dropping the row: unlike a shape + # event, a usage event without a project is still a real pull, + # and discarding it would deflate the pull-through this table + # exists to report. Null already means "not reported". + project_id=(project_id_map.get(ev["project_id"]) + if ev.get("project_id") else None), created_at=_dt(ev.get("created_at")), )) stats["note_usage_events"] += 1 diff --git a/tests/test_note_usage.py b/tests/test_note_usage.py index 5ab69cf..26ba708 100644 --- a/tests/test_note_usage.py +++ b/tests/test_note_usage.py @@ -41,7 +41,8 @@ async def test_record_pulled_writes_a_single_row(): record_pulled(user_id=7, note_id=11, source="mcp_get_snippet") rows = sched.call_args[0][0] assert rows == [ - {"user_id": 7, "note_id": 11, "event": "pulled", "source": "mcp_get_snippet"} + {"user_id": 7, "note_id": 11, "event": "pulled", + "source": "mcp_get_snippet", "project_id": None} ] diff --git a/tests/test_services_backup.py b/tests/test_services_backup.py index 40ed6f2..33e63e9 100644 --- a/tests/test_services_backup.py +++ b/tests/test_services_backup.py @@ -245,6 +245,42 @@ def test_every_column_is_exported_or_declared_excluded(table): ) +def test_the_usage_importer_restores_the_reading_project(): + """The column guard above checks the EXPORT side only. + + A column can be exported faithfully and then dropped on the way back in, + which restores a backup that reports success and has quietly lost a + dimension — #3182's failure mode, one direction over. There is no general + import-side guard yet; this covers the column #4196 added, by source + inspection, because the behavioural path needs Postgres. + + It also pins the DEGRADE. `code_shape_events` skips a row whose project + will not map, because a shape event without its project says nothing. A + usage event is not like that: the project is optional by design and null + already means "not reported", so an unmappable one must restore as + unreported rather than vanish — dropping it would lose a real pull and + deflate the very pull-through this table exists to report. + """ + import inspect + + src = inspect.getsource(backup._restore_v2) + marker = 'for ev in data.get("note_usage_events", []):' + assert marker in src, "the usage import loop moved; this guard is blind" + block = src[src.index(marker):][:1200] + + assert 'ev.get("project_id")' in block, ( + "the usage importer drops project_id — a restore would report success " + "and come back without the reading project" + ) + assert "project_id_map" in block, ( + "project_id must be re-mapped; a raw id points at whatever project " + "happens to hold that number in the destination install" + ) + assert "continue" not in block.split('project_id=')[1][:200], ( + "an unmappable project must degrade to None, not skip the row" + ) + + def test_the_column_guard_covers_every_table_with_a_row_helper(): """The guard is only as good as its registry — a table added to _BACKED_UP with a new helper, and not to the registry, would be unguarded and look