correlator+summarizer: per-scene call-doc storage, fixes #96 and #114's real fix #132

Merged
logan merged 2 commits from fix/96-114-per-scene-call-doc into main 2026-09-13 13:34:19 -04:00
5 changed files with 118 additions and 2 deletions
Showing only changes of commit 0fe6d3b567 - Show all commits
+9
View File
@@ -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)
# ---------------------------------------------------------------------------
@@ -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.<scene_index>. 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.
+9
View File
@@ -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.
+4
View File
@@ -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()
@@ -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