corr_debug is overwritten per scene on the shared call doc, so #35's "LLM tier decides 3.5% of links" is a per-call blend and cannot answer whether the tier works #96

Closed
opened 2026-09-01 02:43:34 -04:00 by logan · 2 comments
Owner

Found by the standing P0 correlation static pass, unattended run 2026-09-01, at b722223. New since the CORRELATION_REVIEW_0820.md baseline — the baseline's §7 complaint was that these fields were not exposed at all, and that part is now fixed at admin.py:134-136. This is the next layer of the same problem: they are exposed, but they are not trustworthy.

The defect

Every scene of a multi-scene call writes corr_debug onto the same call doc (incident_correlator.py:1321), with partial keys, last-write-wins. A call carrying three scenes ends up with corr_path from whichever scene wrote last and corr_consensus / corr_llm_action / corr_llm_reason from a different one. The record does not describe any single correlation decision — it is a splice of several.

admin.py:292-295 then tallies these per call.

Why this matters more than a cosmetic debug bug

It invalidates the measurement #35 is built on. #35 states the LLM correlation tier decides only 3.5% of links and that the fast/thin path dominates. That number is computed off a per-call blend, so it is not a per-link rate and cannot be converted into one after the fact. The tier could be deciding materially more or materially less; the instrument cannot tell us.

Consequences that follow:

  • #35 cannot be actioned on its current evidence. Any tuning aimed at raising the LLM tier's share would be steering by a broken gauge.
  • The standing evaluation procedure (#57) and the correlation dumps saved per run compare corr_path distribution across runs. For multi-scene calls that trend line is noise, and multi-scene calls are exactly the population where over-split (#5) and over-merge live.
  • It hides which scene produced which decision, which is the single fact needed to debug an over-split.

Fix shape

corr_debug should be per scene, keyed by scene index, not a single map on the call doc — e.g. a list or a map keyed by scene id, each entry complete in itself. admin.py then tallies per scene, which is the unit correlation actually operates on. Once that lands, #35's 3.5% figure must be re-measured before anything is concluded from it, and this issue should be referenced when it is.

Cheap interim option if the schema change is too large for one pass: write the scene index into each corr_debug entry and have the tally count only single-scene calls, so at least the reported rate is honest about its population.

Test gap

Nothing covers the multi-scene corr_debug write. drb-c2-core is 290 pass / 0 fail at HEAD; this path is outside the tested gate and dedup paths, like the other two findings from this pass.

Related: #35 (its metric is the thing invalidated), #57, #80, #5. Comment left on #35 so its number is not read at face value in the meantime.

Found by the standing P0 correlation static pass, unattended run 2026-09-01, at `b722223`. **New since the `CORRELATION_REVIEW_0820.md` baseline** — the baseline's §7 complaint was that these fields were not exposed at all, and that part is now fixed at `admin.py:134-136`. This is the next layer of the same problem: they are exposed, but they are not trustworthy. ## The defect Every scene of a multi-scene call writes `corr_debug` onto **the same call doc** (`incident_correlator.py:1321`), with partial keys, last-write-wins. A call carrying three scenes ends up with `corr_path` from whichever scene wrote last and `corr_consensus` / `corr_llm_action` / `corr_llm_reason` from a different one. The record does not describe any single correlation decision — it is a splice of several. `admin.py:292-295` then tallies these **per call**. ## Why this matters more than a cosmetic debug bug **It invalidates the measurement #35 is built on.** #35 states the LLM correlation tier decides only 3.5% of links and that the fast/thin path dominates. That number is computed off a per-call blend, so it is not a per-link rate and cannot be converted into one after the fact. The tier could be deciding materially more or materially less; the instrument cannot tell us. Consequences that follow: - **#35 cannot be actioned on its current evidence.** Any tuning aimed at raising the LLM tier's share would be steering by a broken gauge. - The standing evaluation procedure (#57) and the correlation dumps saved per run compare `corr_path` distribution across runs. For multi-scene calls that trend line is noise, and multi-scene calls are exactly the population where over-split (#5) and over-merge live. - It hides which scene produced which decision, which is the single fact needed to debug an over-split. ## Fix shape `corr_debug` should be **per scene, keyed by scene index**, not a single map on the call doc — e.g. a list or a map keyed by scene id, each entry complete in itself. `admin.py` then tallies per scene, which is the unit correlation actually operates on. Once that lands, **#35's 3.5% figure must be re-measured before anything is concluded from it**, and this issue should be referenced when it is. Cheap interim option if the schema change is too large for one pass: write the scene index into each `corr_debug` entry and have the tally count only single-scene calls, so at least the reported rate is honest about its population. ## Test gap Nothing covers the multi-scene `corr_debug` write. `drb-c2-core` is 290 pass / 0 fail at HEAD; this path is outside the tested gate and dedup paths, like the other two findings from this pass. Related: #35 (its metric is the thing invalidated), #57, #80, #5. Comment left on #35 so its number is not read at face value in the meantime.
Author
Owner

Fixed in #132 (fix/96-114-per-scene-call-doc).

Added a per-scene scenes.<scene_index> map on the call doc (incident_correlator._apply_and_log, additive, doc_set(..., merge=True) — confirmed Firestore recursively merges nested maps, so each scene's write lands in its own map key instead of colliding). Each entry carries that scene's own transcript, the incident_id it resolved to, and its full corr_debug. The existing flat corr_* fields are left exactly as they were (last-scene-wins) for backward compatibility.

admin.py's _call_summary now exposes a scenes list per call, and the summary tally iterates scenes-if-present else falls back to the flat fields for old-schema docs — a 2-scene call now counts as 2 data points instead of 1 blended one. New scene_decision_count next to linked_call_count makes the distinction visible.

Per the fix-shape note in the issue: #35's 3.5% figure should be re-measured now that the tally is honest about its population — flagging that here since this issue is what invalidated it.

14 new tests, full suite 364 -> 378 passed.

Fixed in #132 (fix/96-114-per-scene-call-doc). Added a per-scene `scenes.<scene_index>` map on the call doc (`incident_correlator._apply_and_log`, additive, `doc_set(..., merge=True)` — confirmed Firestore recursively merges nested maps, so each scene's write lands in its own map key instead of colliding). Each entry carries that scene's own `transcript`, the `incident_id` it resolved to, and its full `corr_debug`. The existing flat `corr_*` fields are left exactly as they were (last-scene-wins) for backward compatibility. `admin.py`'s `_call_summary` now exposes a `scenes` list per call, and the summary tally iterates scenes-if-present else falls back to the flat fields for old-schema docs — a 2-scene call now counts as 2 data points instead of 1 blended one. New `scene_decision_count` next to `linked_call_count` makes the distinction visible. Per the fix-shape note in the issue: **#35's 3.5% figure should be re-measured** now that the tally is honest about its population — flagging that here since this issue is what invalidated it. 14 new tests, full suite 364 -> 378 passed.
Author
Owner

Review found and fixed one blocker (commit 0fe6d3b):

Blocker — stale scenes entries survived re-extraction. PATCH /calls/{id}/transcript wipes tags/severity/location/units/embedding before re-running extraction, but not scenes, and doc_set(merge=True) can only add/overwrite nested map keys, never remove one. A call corrected from 3 scenes down 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 named a real incident. Fixed with fstore.doc_update(..., {"scenes": fstore.DELETE_FIELD}) — a real delete, not a merge over {}. Added fstore.DELETE_FIELD (re-exports the real sentinel) and a matching stub entry in tests/conftest.py, which didn't have one. New test: tests/test_reprocess_clears_stale_scenes.py.

Also softened an overclaiming docstring — the Firestore nested-merge behavior is verified against the doc_set wrapper's pass-through code and Firestore's documented contract, not against a live Firestore instance (no SDK available in any sandbox this landed from).

Sandboxed pytest: 378 → 380, all green.

Not fixed, noted instead (non-blocking, safe by construction): the manual-attach endpoint and any future #131 relink both skip writing a scenes entry. summarizer._scene_text_for_incident falls back to whole-call transcript_corrected or transcript when no scene matches — the old #114 behavior, not an error, no data loss, just not the full per-scene fix on those two paths. Worth a follow-up if #131's relink bug turns out to be common.

Review found and fixed one blocker (commit `0fe6d3b`): **Blocker — stale `scenes` entries survived re-extraction.** `PATCH /calls/{id}/transcript` wipes `tags`/`severity`/`location`/`units`/`embedding` before re-running extraction, but not `scenes`, and `doc_set(merge=True)` can only add/overwrite nested map keys, never remove one. A call corrected from 3 scenes down to 1 kept `scenes.1`/`scenes.2` with pre-correction transcripts and `incident_id`s 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 named a real incident. Fixed with `fstore.doc_update(..., {"scenes": fstore.DELETE_FIELD})` — a real delete, not a merge over `{}`. Added `fstore.DELETE_FIELD` (re-exports the real sentinel) and a matching stub entry in `tests/conftest.py`, which didn't have one. New test: `tests/test_reprocess_clears_stale_scenes.py`. Also softened an overclaiming docstring — the Firestore nested-merge behavior is verified against the `doc_set` wrapper's pass-through code and Firestore's documented contract, not against a live Firestore instance (no SDK available in any sandbox this landed from). Sandboxed pytest: 378 → 380, all green. **Not fixed, noted instead (non-blocking, safe by construction):** the manual-attach endpoint and any future #131 relink both skip writing a `scenes` entry. `summarizer._scene_text_for_incident` falls back to whole-call `transcript_corrected or transcript` when no scene matches — the old #114 behavior, not an error, no data loss, just not the full per-scene fix on those two paths. Worth a follow-up if #131's relink bug turns out to be common.
logan closed this issue 2026-09-13 13:34:19 -04:00
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: logan/server-26#96