No reviewers
Labels
No labels
blocked
bug
enhancement
high-priority
low-priority
needs-info
needs-triage
ready-for-agent
ready-for-human
referenced
research
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gabogg/hikcentral!54
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/counting-kpi-group-join-cutover"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #28
Closes #35
Phase 2 of 2. 🔒 Blocked: waits for #53 to merge. Can proceed in parallel with #52 once #53 lands.
This description replaces the 2026-09-22 draft. The second grilling session (2026-09-22/23) found that every camera group in this deployment has exactly one camera, so the drafted full group-join cutover would change nothing observable while costing a table rebuild, seven query rewrites and a user-facing relabel. That work moved to #62, triggered by the first multi-camera group. This PR keeps only what is wrong today.
1. Trust rules and KPIs read the same dataset (#35)
Five aggregates inner-join
counting_cameraswithc.is_excluded = 0 AND c.is_active = 1(occupancy_repository.pyget_counts_in_range_async,get_timespan_aggregates_async,get_cycle_peak_occupancy_async,get_cycle_average_occupancy_async,get_bucketed_cycle_flow_async).get_hourly_flow_distribution_asynchas no join and no filter, so it sums excluded cameras, inactive cameras and orphan rows. It is the sole input to trust rules R1/R2 inevaluate_cycle_integrity_async: the engine deciding whether a cycle is trustworthy reads a different dataset from the one published.get_hourly_flow_distribution_asyncthe same join and filter as the KPI aggregates.get_timespan_aggregates_sync(:741) — dead code, no callers.2. Documentation of the 1:1 invariant (#28)
Already pushed on this branch (
a663cb2):CONTEXT.md§3 — Camera Group (the HikCentral resource group;_Avoid_: Zone), the one-camera-per-group invariant, Direction (IN/OUT, per event) vs DirectionType (ENTRANCE/EXIT/BIDIRECTIONAL, per camera).The runtime guard that flags multi-camera groups is in #53. With the invariant documented and violations visible, #28's fabricated split can no longer happen silently, so #28 closes here; the multi-camera case is #62.
Acceptance criteria
get_hourly_flow_distribution_asyncapplies the same join and exclusion filter as the KPI aggregates.get_timespan_aggregates_syncremoved.CONTEXT.md§3 and ADR 0005 committed.masterafter #53.node --test).🤖 Generated with Claude Code
WIP: fix(occupancy): camera-group join cutover — phase 2 (#28, #35)to WIP: fix(occupancy): trust/KPI dataset parity and 1:1 camera-group invariant — phase 2 (#28, #35)🔒 Blocked — waiting for #53 to merge.
Settled in the grilling session of 2026-09-22/23: merge order is #53 → then #52 and #54 in parallel. #53 rewrites the sync loop (
occupancy_service.py:1814-1888) and deletes the webhook counting block, both of which this PR depends on. Do not start implementation until #53 is merged; then mergemasterinto this branch first.Description updated to the settled scope. The deferred full group-join cutover is #62.
🔓 Unblocked — #53 merged (
1ed0f45, 2026-09-23).Merging
masterinto this branch (it also brings #63's glossary changes) and starting implementation. Draft status removed at the maintainer's request.Scope reminder: #58 waits on this PR, and the full group-join cutover is #62.
WIP: fix(occupancy): trust/KPI dataset parity and 1:1 camera-group invariant — phase 2 (#28, #35)to fix(occupancy): trust/KPI dataset parity and 1:1 camera-group invariant — phase 2 (#28, #35)Implemented —
dfb0644get_hourly_flow_distribution_async(sole input to trust rules R1/R2) now joinscounting_cameraswith the sameis_excluded = 0 AND is_active = 1filter as the KPI aggregates.get_timespan_aggregates_syncdeleted (no callers).mastermerged in (brings #53 and #63;CONTEXT.mdmerged cleanly: Camera Group from this branch sits alongside Camera Group States / Quarantined Counting Event from #53).New
tests/test_trust_dataset_parity.py:FLAG_BURST_COUNTER_FLUSH(confirmed failing against the old query, passing with the fix).The half-open cycle window (
< cycle_end) of the hourly query is unchanged; the KPI query uses<= end_epoch. They differ only for an event stamped exactly at the cycle boundary. That is left for #58, which removes this query entirely.Verification: pytest 233 passed / 1 skipped,
node --test59/59, ruff clean, pre-commit hooks passed. #58 can start once this merges.Code review — round 1
Reviewed
dfb0644againstmaster(4115881) with two independent passes: Standards (docs/standards/code-standards.md,AGENTS.md, domain-modelingCONTEXT-FORMAT.md/ADR-FORMAT.md, plus a code-smell baseline) and Spec (this PR's description, #35, #28 and its amendment). The Spec pass re-ran the suites in a throwaway worktree: pytest 233 passed / 1 skipped,node --test59/59. Mutation check: with the old unfiltered query restored, all 4 new tests fail.Verdict: ready to merge. No P1 on either axis, no P2 on Spec.
Standards
No documented-standard breaches: SQL stays in the repository, annotations intact, tests use real SQLite with nothing mocked.
CONTEXT.md:88vs:159: the invariant paragraph under Camera Group repeats #53's Multi-camera group bullet almost verbatim ("per-camera figures… are estimates, not measurements"). Keep it in one place.occupancy_repository.py:1502-1506:JOIN counting_cameras … AND c.is_excluded = 0 AND c.is_active = 1now appears six times (:973, :994, :1136, :1200, :1506, :1598). #35 exists because copies of this rule drifted; the fix adds another hand copy. Name the concept once.:88: the invariant is a behavioural rule ("The system expects…") inside a definition.:101: the second_Avoid_sentence is a scope note, not an alias.:3:Status:is a body line rather than frontmatter, and the opening context runs past the template's 1–3 sentences. The ADR otherwise meets the criteria, and its multi-camera-flag claim checks out in the code.get_timespan_aggregates_syncis safe (no callers) but outside the #35 commit's stated scope.@pytest.mark.asynciois redundant underasyncio_mode = "auto"(§4.2), though consistent with the suite.Spec
All KPI aggregates and the trust input now share the same join and filter; R3/R4 are fed filtered totals at both call sites of
evaluate_cycle_integrity_async. #35's fixes (1) and (2) are done, (3) is #58. #28 closing is consistent with the rescope (1:1 invariant, #53's guard, #62). No scope creep./simulate(occupancy_controller.py:385-386,direction_type … or cams) into phase 2; the rescoped PR neither fixes nor explicitly defers it. It is already item 7 of #62's scope.< cycle_end, the KPI queries at<= end. Only events in a cycle's final millisecond differ, and that can only flip R2 on a cycle sitting at the 35% line. Declared as left to #58.get_earliest_event_epoch_asyncandget_passenger_flow_telemetry_asyncare still unfiltered. Neither feeds a KPI or a trust rule.Standards: 6 findings, worst P2 (glossary overlap). Spec: 3 findings, worst P3. All being addressed in a follow-up commit on this branch.
Review round 1 addressed —
990d495Standards
_COUNTED_CAMERA_JOIN/_COUNTED_CAMERA_FILTERinoccupancy_repository.py, with a comment naming the concept. All six queries use them._Avoid_line no longer carries a scope note.status/datefrontmatter, a two-sentence summary, and the detail moved into a Context section.get_timespan_aggregates_syncis now called out in the follow-up commit message and was already in this PR's description.@pytest.mark.asyncio: fixed in this PR's test file (asyncio_mode = "auto").Spec
<vs<=window: fixed. The hourly trust input now ends at<= cycle_end, like the KPI queries. New testtest_window_bounds_match_at_the_cycle_edgespins both edges. (#58 will move trust toget_bucketed_cycle_flow_async, which still uses<; #58 must keep this parity.)get_earliest_event_epoch_asynconly bounds where reconciliation starts;get_passenger_flow_telemetry_asyncis the raw audit, export and recent-passages log. Both docstrings now say why the counted-camera rule doesn't apply./simulatedirection derivation: stays deferred; it is item 7 of #62's scope ("Simulator… drop itsor camsfallback").Verification: pytest 234 passed / 1 skipped,
tests/test_docs.py7/7, ruff clean, pre-commit hooks passed.Code review — round 2 (follow-up)
Reviewed
990d495againstmasterwith the same two independent passes. CI green; merges cleanly.Verdict: ready to merge. Every round-1 finding holds; no P1 or P2 on either axis.
Standards
Round 1: all 6 hold (glossary overlap, one counted-camera rule, CONTEXT-FORMAT, ADR-FORMAT, commit scope, markers). Interpolating the
_COUNTED_CAMERA_*constants into f-string SQL is compliant: code-standards only requires SQL in repositories with values bound via?, which they are. No query had literal braces, and nothing parsesdocs/adr.:1061,:1084). Pre-existing third copy.get_bucketed_cycle_flow_asyncstill ends at<(:1608) while the hourly query now ends at<=.hourandatfor the same timestamp.Spec
Round 1: all 3 hold. Real callers pass
cycle_endasreset - 0.001,start + 86400 - 0.001ort; a probe gave buckets 0–23 only, so R1/R2 are unaffected by<=. Five of the refactored queries produce byte-identical SQL, and the sixth differs only by the intended<=. pytest 234 passed; a mutation back to<fails the boundary test.<=parity" caveat lived only in a PR comment, not in #58 or the triage record.start + 24 hexactly, which no caller passes; the end event lands in a 25th bucket the volume-sum assertion can't see.Standards: 4 findings, worst P3. Spec: 2 findings, worst P3. All filed in #66; the parity caveat is also posted on #58. Merging.