From 0fe6d3b567dfc7df9aaea66f29733e7ee9867ec9 Mon Sep 17 00:00:00 2001 From: Logan Cusano Date: Sun, 13 Sep 2026 13:33:52 -0400 Subject: [PATCH] correlator: delete stale scenes on re-extraction instead of leaving them to rot (#96/#114) Review of #132 found a blocker: PATCH /calls/{id}/transcript wipes tags/severity/location/units/embedding before re-extraction, but not the new scenes map, and doc_set(merge=True) can only add/overwrite nested map keys, never remove one. A call corrected from 3 scenes to 1 kept scenes.1/scenes.2 with pre-correction transcripts and incident_ids forever -- corrupting the exact per-scene tally #96 exists to make trustworthy, and able to re-feed stale text into #114's summarizer fix if a stale scene's incident_id still names a real incident. Fix: fstore.doc_update(...,{"scenes": fstore.DELETE_FIELD}) -- a real delete, not a merge over an empty map. Added fstore.DELETE_FIELD (re-exports the real firebase_admin sentinel) and stubbed it in the sandboxed test conftest, which didn't have it. Also softened an overclaiming docstring: the Firestore nested-merge behavior is verified against the doc_set wrapper's pass-through, not against live Firestore. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Tbknwttzou4s46PAykmtix --- drb-c2-core/app/internal/firestore.py | 9 ++ .../app/internal/incident_correlator.py | 9 +- drb-c2-core/app/routers/calls.py | 9 ++ drb-c2-core/tests/conftest.py | 4 + .../test_reprocess_clears_stale_scenes.py | 89 +++++++++++++++++++ 5 files changed, 118 insertions(+), 2 deletions(-) create mode 100644 drb-c2-core/tests/test_reprocess_clears_stale_scenes.py diff --git a/drb-c2-core/app/internal/firestore.py b/drb-c2-core/app/internal/firestore.py index c3ab551..c9415c2 100644 --- a/drb-c2-core/app/internal/firestore.py +++ b/drb-c2-core/app/internal/firestore.py @@ -7,6 +7,15 @@ from google.cloud.firestore_v1.base_query import FieldFilter from app.config import settings from app.internal.logger import logger +# Re-exported so callers never need their own `firebase_admin.firestore` import +# just to delete a field. server-26#96/#114 review: `doc_set(..., merge=True)` +# merges nested maps by key but can never REMOVE one — writing `{"scenes": {}}` +# to clear a map is a no-op, not a delete. Use `doc_update(coll, id, {"field": +# fstore.DELETE_FIELD})` (or doc_set + merge, DELETE_FIELD works under both) +# whenever a re-extraction/reprocess path needs a stale nested field gone +# rather than merged over. +DELETE_FIELD = fs.DELETE_FIELD + # --------------------------------------------------------------------------- # In-memory TTL cache for rarely-changing documents (systems, nodes config) # --------------------------------------------------------------------------- diff --git a/drb-c2-core/app/internal/incident_correlator.py b/drb-c2-core/app/internal/incident_correlator.py index 5de12f7..3ea816a 100644 --- a/drb-c2-core/app/internal/incident_correlator.py +++ b/drb-c2-core/app/internal/incident_correlator.py @@ -1458,8 +1458,13 @@ async def _apply_and_log(decision: dict, ctx: dict) -> Optional[str]: same corr_debug — plus this scene's own transcript and the incident_id it resolved to — under scenes.. Firestore's `DocumentReference.set(data, merge=True)` recursively merges nested map - fields by key (confirmed against the documented set-with-merge semantics, - not assumed): a write of {"scenes": {"1": {...}}} merges into an existing + fields by key. Verified against `internal/firestore.py`'s `doc_set` + wrapper (a straight `ref.set(data, merge=merge)` pass-through — no + `update()`, no read-modify-write, nothing that would change this) and + against the documented set-with-merge semantics; NOT exercised against a + live Firestore instance (no SDK available in the sandboxes this landed + from — review flagged this distinction explicitly). A write of + {"scenes": {"1": {...}}} merges into an existing {"scenes": {"0": {...}}} to produce {"scenes": {"0": {...}, "1": {...}}} rather than replacing the whole `scenes` map, so scene 0's and scene 1's entries land side by side instead of colliding like the flat fields do. diff --git a/drb-c2-core/app/routers/calls.py b/drb-c2-core/app/routers/calls.py index 9663981..b3efaa3 100644 --- a/drb-c2-core/app/routers/calls.py +++ b/drb-c2-core/app/routers/calls.py @@ -260,6 +260,15 @@ async def patch_transcript( "vehicles": [], "embedding": None, }) + # server-26#96/#114 review: doc_set(merge=True) can only ADD/overwrite keys + # in a nested map, never remove one, so the fields above get cleared but a + # prior `scenes` map would survive re-extraction forever. A call corrected + # from 3 scenes down to 1 would keep scenes.1/scenes.2 with pre-correction + # transcripts and incident_ids -- corrupting the exact per-scene tally #96 + # exists to make trustworthy, and re-feeding stale text into #114's + # summarizer fix if a stale scene's incident_id still names a real + # incident. Must be a real delete, not a merge over an empty map. + await fstore.doc_update("calls", call_id, {"scenes": fstore.DELETE_FIELD}) # Unlink from ALL current incidents so re-correlation starts clean. # Handles both old single incident_id and new incident_ids list. diff --git a/drb-c2-core/tests/conftest.py b/drb-c2-core/tests/conftest.py index 7364d68..41c939f 100644 --- a/drb-c2-core/tests/conftest.py +++ b/drb-c2-core/tests/conftest.py @@ -34,6 +34,10 @@ except ModuleNotFoundError: # into dicts that tests compare against, and a MagicMock compares unequal # to itself across attribute accesses. _fs.SERVER_TIMESTAMP = "__SERVER_TIMESTAMP__" + # Same reasoning as SERVER_TIMESTAMP above: a distinct sentinel, not a + # MagicMock, so `fstore.DELETE_FIELD is fs.DELETE_FIELD` and dict/`is` + # comparisons against it in tests (server-26#96/#114, PR #132) behave. + _fs.DELETE_FIELD = "__DELETE_FIELD__" _auth = ModuleType("firebase_admin.auth") _auth.verify_id_token = MagicMock() diff --git a/drb-c2-core/tests/test_reprocess_clears_stale_scenes.py b/drb-c2-core/tests/test_reprocess_clears_stale_scenes.py new file mode 100644 index 0000000..92477a8 --- /dev/null +++ b/drb-c2-core/tests/test_reprocess_clears_stale_scenes.py @@ -0,0 +1,89 @@ +""" +server-26#96/#114 review (PR #132): `PATCH /calls/{id}/transcript` clears +stale intelligence fields before re-extraction runs, but `doc_set(..., +merge=True)` can only add/overwrite keys in a nested map, never remove one. +A call corrected from 3 scenes down to 1 would keep `scenes.1`/`scenes.2` +with pre-correction transcripts and incident_ids forever -- corrupting the +per-scene tally #96 exists to make trustworthy, and re-feeding stale text +into #114's summarizer fix if a stale scene's incident_id still names a real +incident. The fix deletes the field with `fstore.DELETE_FIELD` instead of +merging over it with an empty map (which is a no-op). +""" +from unittest.mock import AsyncMock, patch + +import pytest +from fastapi import BackgroundTasks + +from app.internal import firestore as fstore +from app.routers.calls import TranscriptUpdate, patch_transcript + + +@pytest.mark.asyncio +async def test_transcript_correction_deletes_the_scenes_field_not_merges_over_it(): + call = { + "call_id": "call-1", + "system_id": "sys-1", + "node_id": "node-1", + "transcript": "old raw text", + # Simulates a prior 3-scene call, per #96's schema. + "scenes": { + "0": {"transcript": "scene zero", "incident_id": "inc-a", "corr_debug": {}}, + "1": {"transcript": "scene one", "incident_id": "inc-b", "corr_debug": {}}, + }, + } + + doc_set_calls: list[tuple] = [] + doc_update_calls: list[tuple] = [] + + async def fake_doc_get(collection, doc_id): + if collection == "calls" and doc_id == "call-1": + return call + return None + + async def fake_doc_set(collection, doc_id, data, merge=True): + doc_set_calls.append((collection, doc_id, data)) + + async def fake_doc_update(collection, doc_id, data): + doc_update_calls.append((collection, doc_id, data)) + + fake_flags = (None, lambda name: name == "correlation_enabled") + + with patch("app.routers.calls.fstore.doc_get", new=fake_doc_get), \ + patch("app.routers.calls.fstore.doc_set", new=fake_doc_set), \ + patch("app.routers.calls.fstore.doc_update", new=fake_doc_update), \ + patch("app.internal.feature_flags.resolve_flags", new=AsyncMock(return_value=fake_flags)): + result = await patch_transcript( + call_id="call-1", + body=TranscriptUpdate(transcript="corrected text"), + background_tasks=BackgroundTasks(), + _={}, + ) + + assert result == {"ok": True, "call_id": "call-1"} + + # The stale scenes map must be DELETED, not merged over with {} (a no-op + # under Firestore's set(merge=True) semantics) and not left untouched by + # a doc_set call that never mentions it. + scenes_deletions = [ + (coll, doc_id, data) for (coll, doc_id, data) in doc_update_calls + if coll == "calls" and doc_id == "call-1" and "scenes" in data + ] + assert len(scenes_deletions) == 1, ( + f"expected exactly one doc_update clearing 'scenes', got {doc_update_calls}" + ) + assert scenes_deletions[0][2]["scenes"] is fstore.DELETE_FIELD + + # And no doc_set call should paper over the same field with an empty map + # instead -- that would silently do nothing and leave stale scenes intact. + for (coll, doc_id, data) in doc_set_calls: + if coll == "calls" and doc_id == "call-1": + assert "scenes" not in data, ( + "a doc_set (merge=True) write must never carry 'scenes' -- " + "merging {} over an existing map is a no-op, not a delete" + ) + + +def test_delete_field_is_the_real_firestore_sentinel(): + """Catches an import-path typo turning this into a silent no-op sentinel.""" + from firebase_admin import firestore as fs + assert fstore.DELETE_FIELD is fs.DELETE_FIELD