gabogg/hikcentral
Watch
1
0
Fork
You've already forked hikcentral
 0
Code Issues 112 Pull requests 9 Projects Releases Packages Wiki Activity Actions

fix(occupancy): camera-group schema, orphan guard and migration — phase 1 (#29) #53

Merged
gabogg merged 6 commits from fix/counting-events-camera-integrity into master 2026-09-23 17:51:11 +00:00
Conversation 9 Commits 6 Files changed 11 +1041 -76
gabogg commented 2026-09-22 15:22:42 +00:00
Owner
Copy link

Closes #29

Phase 1 of 2. Merges first — #52 and #54 are blocked on this PR. Supersedes #51 (closed as duplicate of #29).

This description replaces the 2026-09-22 draft following the second grilling session (2026-09-22/23). Changes versus that draft: a second fallback and a webhook bug are now in scope; the schema shrinks (no group-level events, no nullable-column rebuild, no needs_confirmation); orphan rows are quarantined, not re-attributed.

Context the design rests on

  • Every camera group in this deployment has exactly one camera (12 active groups ↔ 12 active cameras; zone_name already carries the group name, occupancy_service.py:423). The even split divides by 1, so per-camera figures are exact. The system now requires this — see CONTEXT.md §3 Camera Group and ADR 0005 (both on #54). The full group-join cutover is deferred to #62.
  • There is no per-crossing counting path. 131585–131588 are door alarms (models.py:111-114, CONTEXT.md §2), not passages.

Invariant this PR enforces

Every counting event's camera_index_code is a registered camera.

Three writers currently break it:

1. Sync fallback A — occupancy_service.py:1830-1831

"in_cams":  in_cams  or [(g_code, g_name)],
"out_cams": out_cams or [(g_code, g_name)],

2. Sync fallback B — occupancy_service.py:1880-1888

A group present in count_list but absent from group_list falls back to the group code via group_cam_map.get(g_code, {...}) and info.get("in_cams") or [(g_code, ...)].

Both write rows under a group code. Those rows never match counting_cameras, so the reconciliation baseline reads 0 and the full cumulative Artemis count is re-injected on every 3-second poll (#29).

3. Webhook misread — webhook_controller.py:31-56

The controller treats 131585/131586 as "Entering/Exiting" and 131588 as a "realtime counting report". They are ALARM_FORCED_OPEN, ALARM_TIMEOUT and RESTORED. Every forced-open or timeout door alarm received by webhook writes a fake people-counting passage under a door code. Those rows are invisible to the KPIs but are read by get_hourly_flow_distribution_async (trust rules R1/R2, #35) and appear in the operator flux stream. Introduced in ce0ff5d (2026-09-03).

Scope

Guard

  • Delete both sync fallbacks.
  • A group with zero member cameras, or present in count_list but not group_list, is skipped entirely: flag set on its counting_camera_groups row, one warning — not a growing table. Nothing attributable is lost: there is no camera to attribute to.
  • Delete the counting block in webhook_controller.py. Door handling and SUBSCRIBED_EVENT_TYPES stay (doors need those codes).
  • The name-heuristic case (a direction list emptied by ENTRADA/SALIDA/ACCESO matching) does not occur with this facility's camera names (all 14 register as BIDIRECTIONAL), and is removed wholesale by #62. No per-direction handling.

1:1 invariant guard

  • A group with more than one camera is flagged on counting_camera_groups, warned in the log, and surfaced to operators (health / config indicator).
  • It keeps counting with the existing even split, so facility totals stay correct; only that group's per-camera split is an estimate. Refusing to count was rejected: it would undercount totals.

Schema (additive only)

  • New counting_camera_groups: resource_group_code (PK), resource_group_name, member_count, empty-group flag, multi-camera flag, timestamps. Persisted from Artemis on each sync. CREATE TABLE IF NOT EXISTS + in-place migration, following database.py:271-276.
  • counting_cameras.resource_group_code — needed to compute group size, and by #62 later.
  • New quarantine table for orphan rows (same columns as people_counting_events plus quarantined_at and reason).
  • Not in this PR (moved to #62): people_counting_events.resource_group_code, nullable camera_index_code (SQLite table rebuild), needs_confirmation.

Orphan migration

Orphan rows are not re-attributable: each runaway row holds the whole cumulative Artemis count at one poll, so summing them is meaningless.

  • Move every people_counting_events row whose camera_index_code matches no counting_cameras row (group codes, door codes, anything else) into the quarantine table, with a reason (group_code / door_code / unknown).
  • Log the counts moved, per reason.
  • Published KPIs do not move (they never saw these rows). The hourly distribution does change, since it summed them — that is the intended correction, and #54 aligns it with the KPIs.
  • Reversible: a later reconstruction (e.g. max cumulative per cycle and direction) can be done from the quarantine table if ever wanted.

Integrity check

Startup and periodic:

SELECT DISTINCT camera_index_code FROM people_counting_events
WHERE camera_index_code NOT IN (SELECT camera_index_code FROM counting_cameras)

Log loudly when non-empty.

Out of scope

Group-level events and the group join (#62) · #35 dataset parity and ADR 0005 (#54) · the WebSocket batching (#52).

Acceptance criteria

  • Both sync fallbacks (:1830-1831, :1880-1888) removed.
  • webhook_controller.py no longer writes counting events; door webhook handling unchanged.
  • Zero-camera groups and groups missing from group_list emit zero events, set their flag, and log one warning.
  • Multi-camera groups keep counting, are flagged, warned, and exposed via GET /api/occupancy/camera-groups (no UI indicator yet — see implementation notes).
  • counting_camera_groups, counting_cameras.resource_group_code and the quarantine table created via CREATE TABLE and in-place migration; existing databases upgrade without loss.
  • All existing orphan rows quarantined with a reason; counts per reason logged.
  • Published KPI figures unchanged before/after migration.
  • Integrity check runs at startup and periodically.
  • Tests: empty relatedResourceInfoList → zero events + one warning; group in count_list but not group_list → zero events; webhook 131585/131586 payload → zero counting events; two-camera group → counted, flagged; migration quarantines a group-code row and a door-code row.
  • Production validation checklist (comment below) run once the prod DB is available.
  • Full suite green (pytest + node --test).

🤖 Generated with Claude Code

Closes #29 Phase 1 of 2. **Merges first — #52 and #54 are blocked on this PR.** Supersedes #51 (closed as duplicate of #29). This description replaces the 2026-09-22 draft following the second grilling session (2026-09-22/23). Changes versus that draft: a second fallback and a webhook bug are now in scope; the schema shrinks (no group-level events, no nullable-column rebuild, no `needs_confirmation`); orphan rows are quarantined, not re-attributed. --- ## Context the design rests on - **Every camera group in this deployment has exactly one camera** (12 active groups ↔ 12 active cameras; `zone_name` already carries the group name, `occupancy_service.py:423`). The even split divides by 1, so per-camera figures are exact. The system now **requires** this — see `CONTEXT.md` §3 *Camera Group* and ADR 0005 (both on #54). The full group-join cutover is deferred to #62. - **There is no per-crossing counting path.** 131585–131588 are **door alarms** (`models.py:111-114`, `CONTEXT.md` §2), not passages. --- ## Invariant this PR enforces > **Every counting event's `camera_index_code` is a registered camera.** Three writers currently break it: ### 1. Sync fallback A — `occupancy_service.py:1830-1831` ```python "in_cams": in_cams or [(g_code, g_name)], "out_cams": out_cams or [(g_code, g_name)], ``` ### 2. Sync fallback B — `occupancy_service.py:1880-1888` A group present in `count_list` but absent from `group_list` falls back to the group code via `group_cam_map.get(g_code, {...})` **and** `info.get("in_cams") or [(g_code, ...)]`. Both write rows under a group code. Those rows never match `counting_cameras`, so the reconciliation baseline reads `0` and the **full cumulative Artemis count is re-injected on every 3-second poll** (#29). ### 3. Webhook misread — `webhook_controller.py:31-56` The controller treats 131585/131586 as "Entering/Exiting" and 131588 as a "realtime counting report". They are `ALARM_FORCED_OPEN`, `ALARM_TIMEOUT` and `RESTORED`. **Every forced-open or timeout door alarm received by webhook writes a fake people-counting passage under a door code.** Those rows are invisible to the KPIs but are read by `get_hourly_flow_distribution_async` (trust rules R1/R2, #35) and appear in the operator flux stream. Introduced in `ce0ff5d` (2026-09-03). --- ## Scope ### Guard - Delete both sync fallbacks. - A group with **zero member cameras**, or present in `count_list` but not `group_list`, is **skipped entirely**: flag set on its `counting_camera_groups` row, one warning — not a growing table. Nothing attributable is lost: there is no camera to attribute to. - Delete the counting block in `webhook_controller.py`. Door handling and `SUBSCRIBED_EVENT_TYPES` stay (doors need those codes). - The name-heuristic case (a direction list emptied by `ENTRADA`/`SALIDA`/`ACCESO` matching) does not occur with this facility's camera names (all 14 register as `BIDIRECTIONAL`), and is removed wholesale by #62. No per-direction handling. ### 1:1 invariant guard - A group with **more than one camera** is flagged on `counting_camera_groups`, warned in the log, and surfaced to operators (health / config indicator). - It **keeps counting** with the existing even split, so facility totals stay correct; only that group's per-camera split is an estimate. Refusing to count was rejected: it would undercount totals. ### Schema (additive only) - **New `counting_camera_groups`**: `resource_group_code` (PK), `resource_group_name`, `member_count`, empty-group flag, multi-camera flag, timestamps. Persisted from Artemis on each sync. `CREATE TABLE IF NOT EXISTS` + in-place migration, following `database.py:271-276`. - **`counting_cameras.resource_group_code`** — needed to compute group size, and by #62 later. - **New quarantine table** for orphan rows (same columns as `people_counting_events` plus `quarantined_at` and `reason`). - **Not in this PR** (moved to #62): `people_counting_events.resource_group_code`, nullable `camera_index_code` (SQLite table rebuild), `needs_confirmation`. ### Orphan migration Orphan rows are **not** re-attributable: each runaway row holds the **whole cumulative** Artemis count at one poll, so summing them is meaningless. - Move **every** `people_counting_events` row whose `camera_index_code` matches no `counting_cameras` row (group codes, door codes, anything else) into the quarantine table, with a `reason` (`group_code` / `door_code` / `unknown`). - Log the counts moved, per reason. - Published KPIs do not move (they never saw these rows). The **hourly distribution does change**, since it summed them — that is the intended correction, and #54 aligns it with the KPIs. - Reversible: a later reconstruction (e.g. max cumulative per cycle and direction) can be done from the quarantine table if ever wanted. ### Integrity check Startup and periodic: ```sql SELECT DISTINCT camera_index_code FROM people_counting_events WHERE camera_index_code NOT IN (SELECT camera_index_code FROM counting_cameras) ``` Log loudly when non-empty. --- ## Out of scope Group-level events and the group join (#62) · #35 dataset parity and ADR 0005 (#54) · the WebSocket batching (#52). --- ## Acceptance criteria - [x] Both sync fallbacks (`:1830-1831`, `:1880-1888`) removed. - [x] `webhook_controller.py` no longer writes counting events; door webhook handling unchanged. - [x] Zero-camera groups and groups missing from `group_list` emit **zero** events, set their flag, and log one warning. - [x] Multi-camera groups keep counting, are flagged, warned, and exposed via `GET /api/occupancy/camera-groups` *(no UI indicator yet — see implementation notes)*. - [x] `counting_camera_groups`, `counting_cameras.resource_group_code` and the quarantine table created via `CREATE TABLE` and in-place migration; existing databases upgrade without loss. - [x] All existing orphan rows quarantined with a reason; counts per reason logged. - [x] Published KPI figures unchanged before/after migration. - [x] Integrity check runs at startup and periodically. - [x] Tests: empty `relatedResourceInfoList` → zero events + one warning; group in `count_list` but not `group_list` → zero events; webhook 131585/131586 payload → zero counting events; two-camera group → counted, flagged; migration quarantines a group-code row and a door-code row. - [x] Production validation checklist (comment below) run once the prod DB is available. - [x] Full suite green (pytest + `node --test`). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
chore(wip): open draft for counting-events/camera integrity (#28, #29, #35)
Some checks failed
CI / lint-and-test (pull_request) Has been cancelled
Details
b3623a0cd6 
WIP scaffold. See PR description for the combined scope across the three
counting_cameras-join issues; implementation follows.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gabogg changed title from WIP: fix(occupancy): counting-events ↔ counting_cameras integrity (#28, #29, #35) to WIP: fix(occupancy): camera-group schema, orphan guard and migration — phase 1 (#29) 2026-09-22 16:31:05 +00:00
gabogg commented 2026-09-23 13:57:08 +00:00
Author
Owner
Copy link

Production validation checklist

To run read-only against the production DB once WireGuard/SSH access is in place. Results decide nothing already settled, but confirm the assumptions this PR and ADR 0005 rest on.

-- 1. 1:1 invariant: expect ZERO rows. Any row = a multi-camera group; #62's trigger may already have fired.
SELECT zone_name, COUNT(*) FROM counting_cameras WHERE is_active = 1 GROUP BY 1 HAVING COUNT(*) > 1;

-- 2. Orphans by shape (sizes the quarantine migration).
SELECT camera_index_code, direction, COUNT(*), SUM(count), MAX(count),
       datetime(MIN(timestamp_epoch),'unixepoch'), datetime(MAX(timestamp_epoch),'unixepoch')
FROM people_counting_events
WHERE camera_index_code NOT IN (SELECT camera_index_code FROM counting_cameras)
GROUP BY 1, 2;

-- 3. Door alarms misrecorded as passages by the webhook (expect > 0 if any forced-open/timeout alarm arrived since 2026-09-03).
SELECT COUNT(*), SUM(count) FROM people_counting_events
WHERE camera_index_code IN (SELECT door_index_code FROM door_records
                            UNION SELECT door_index_code FROM door_hardware_state_transitions);

-- 4. Any counting row that came through the webhook at all (raw_payload is '{}' for sync rows).
SELECT COUNT(*) FROM people_counting_events WHERE raw_payload LIKE '%eventType%';

Please paste results here. If (1) returns rows, stop and revisit ADR 0005 / #62 before implementing.

## Production validation checklist To run **read-only** against the production DB once WireGuard/SSH access is in place. Results decide nothing already settled, but confirm the assumptions this PR and ADR 0005 rest on. ```sql -- 1. 1:1 invariant: expect ZERO rows. Any row = a multi-camera group; #62's trigger may already have fired. SELECT zone_name, COUNT(*) FROM counting_cameras WHERE is_active = 1 GROUP BY 1 HAVING COUNT(*) > 1; -- 2. Orphans by shape (sizes the quarantine migration). SELECT camera_index_code, direction, COUNT(*), SUM(count), MAX(count), datetime(MIN(timestamp_epoch),'unixepoch'), datetime(MAX(timestamp_epoch),'unixepoch') FROM people_counting_events WHERE camera_index_code NOT IN (SELECT camera_index_code FROM counting_cameras) GROUP BY 1, 2; -- 3. Door alarms misrecorded as passages by the webhook (expect > 0 if any forced-open/timeout alarm arrived since 2026-09-03). SELECT COUNT(*), SUM(count) FROM people_counting_events WHERE camera_index_code IN (SELECT door_index_code FROM door_records UNION SELECT door_index_code FROM door_hardware_state_transitions); -- 4. Any counting row that came through the webhook at all (raw_payload is '{}' for sync rows). SELECT COUNT(*) FROM people_counting_events WHERE raw_payload LIKE '%eventType%'; ``` Please paste results here. If (1) returns rows, stop and revisit ADR 0005 / #62 before implementing.
fix(occupancy): every counting event belongs to a registered camera (#29)
All checks were successful
CI / lint-and-test (pull_request) Successful in 1m13s
Details
ba57b9e6f7 
Enforce the invariant that a people_counting_events row always carries a
registered camera code, and record the one-camera-per-group invariant
(ADR 0005) at runtime.

Sync (occupancy_service.sync_passenger_flow_from_artemis_async):
- Remove both fallbacks that wrote the resource-group code as a camera
  code. Those rows matched no camera, so the reconciliation baseline read
  0 and the full cumulative Artemis count was re-injected every poll.
- Skip groups with no member cameras, groups absent from the group list,
  and groups with a member camera that is not registered/active (same
  runaway via a missing baseline). Warn once per group, not per poll.
- A direction left empty by the name heuristic goes to the group's member
  cameras instead of the group code.

Webhook: stop ingesting 131585-131588 as passages. They are door alarms
(ALARM_FORCED_OPEN, ALARM_TIMEOUT, EMERGENCY_OPEN, RESTORED); every such
alarm was written as a fake counting event under a door code.

Camera groups: new counting_camera_groups table and
counting_cameras.resource_group_code, populated on camera sync. Groups
are flagged is_empty / is_multi_camera; a multi-camera group keeps
counting (totals stay correct) and is warned about once. Exposed via
GET /api/occupancy/camera-groups.

Integrity: orphan events (group, door or unknown codes) are moved to
people_counting_events_quarantine with a reason, at startup and hourly.
They were invisible to every KPI, so published figures do not move.
Skipped while no camera is registered.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-09-23 14:12:51 +00:00
Author
Owner
Copy link

Implementation notes (ba57b9e)

Implemented. Verification: pytest 223 passed / 1 skipped (10 new in tests/test_counting_integrity.py), node --test 59/59, ruff check + ruff format --check clean. Migration run twice against a copy of the local data/hikcentral.db: 3,726 rows / 35,081 passages preserved, new columns and tables present, integrity check found nothing to quarantine.

Where the code differs from the description, and why:

  1. A direction list emptied by the name heuristic now goes to the group's member cameras. The description said "no per-direction handling", but once the fallback is gone something has to happen there, or the division by len(in_cams) fails. Sending that direction to the members is correct under 1:1 groups (the group's only camera measured it), loses no data, and never writes a group code.
  2. New guard: a member camera that is not registered, or is inactive, skips its group. It is the same runaway as #29 by a second route: the baseline sums only registered active cameras, so a missing member reads 0 and gets the whole cumulative count re-injected every poll. The #29 body mentions this case; startup syncs cameras first, so it only affects the ≤60 s window before a new camera is registered.
  3. Quarantine runs at startup and every hour, not only once as a migration. Orphans are invisible to every KPI, so moving them never changes published figures. It is skipped while counting_cameras is empty, because an empty registry would make every row look orphaned.
  4. Operator surface is API-only for now: GET /api/occupancy/camera-groups (operator auth) returns is_empty and is_multi_camera per group, plus a one-time log warning. No dashboard badge; that is frontend work, left for a follow-up if you want it.
  5. The warning for a skipped group fires once, and fires again only if the group recovers and then breaks again.

Still open: the production validation checklist above.

## Implementation notes (`ba57b9e`) Implemented. Verification: **pytest 223 passed / 1 skipped** (10 new in `tests/test_counting_integrity.py`), **`node --test` 59/59**, `ruff check` + `ruff format --check` clean. Migration run twice against a copy of the local `data/hikcentral.db`: 3,726 rows / 35,081 passages preserved, new columns and tables present, integrity check found nothing to quarantine. Where the code differs from the description, and why: 1. **A direction list emptied by the name heuristic now goes to the group's member cameras.** The description said "no per-direction handling", but once the fallback is gone *something* has to happen there, or the division by `len(in_cams)` fails. Sending that direction to the members is correct under 1:1 groups (the group's only camera measured it), loses no data, and never writes a group code. 2. **New guard: a member camera that is not registered, or is inactive, skips its group.** It is the same runaway as #29 by a second route: the baseline sums only registered active cameras, so a missing member reads `0` and gets the whole cumulative count re-injected every poll. The #29 body mentions this case; startup syncs cameras first, so it only affects the ≤60 s window before a new camera is registered. 3. **Quarantine runs at startup and every hour**, not only once as a migration. Orphans are invisible to every KPI, so moving them never changes published figures. It is **skipped while `counting_cameras` is empty**, because an empty registry would make every row look orphaned. 4. **Operator surface is API-only for now:** `GET /api/occupancy/camera-groups` (operator auth) returns `is_empty` and `is_multi_camera` per group, plus a one-time log warning. No dashboard badge; that is frontend work, left for a follow-up if you want it. 5. The warning for a skipped group fires **once**, and fires again only if the group recovers and then breaks again. Still open: the production validation checklist above.
gabogg commented 2026-09-23 14:31:42 +00:00
Author
Owner
Copy link

Production validation — results (2026-09-23)

Run read-only (sqlite3 URI mode=ro) on the production server against the live DB. The dataset holds 595,589 counting events / 1,030,753 passages, 2026-09-03 → 2026-09-23.

# Check Result Meaning
1 Multi-camera groups 0 rows. 12 active groups ↔ 12 active cameras, none excluded The one-camera-per-group invariant (ADR 0005) holds in production. #62's trigger has not fired.
2 Orphan rows 0 No #29 runaway has happened in production. The quarantine migration is a no-op; it stays in as the guard.
3 Door codes recorded as passages 0 The webhook misread is a real bug but it never fired. door_events shows no ALARM_FORCED_OPEN/ALARM_TIMEOUT ever received, and one RESTORED (131588), whose missing enterNum/exitNum wrote nothing.
4 Counting rows from the webhook 0 Confirms counting is 100% polling. The webhook itself works: 3,453 door transitions arrived through it.

Conclusion: no historical data needs correcting. Everything in this PR is preventive, and the design assumptions (1:1 groups, no per-crossing counting path) are confirmed against production.

## Production validation — results (2026-09-23) Run read-only (`sqlite3` URI `mode=ro`) on the production server against the live DB. The dataset holds **595,589 counting events / 1,030,753 passages, 2026-09-03 → 2026-09-23**. | # | Check | Result | Meaning | |---|---|---|---| | 1 | Multi-camera groups | **0 rows.** 12 active groups ↔ 12 active cameras, none excluded | The one-camera-per-group invariant (ADR 0005) holds in production. #62's trigger has not fired. | | 2 | Orphan rows | **0** | No #29 runaway has happened in production. The quarantine migration is a no-op; it stays in as the guard. | | 3 | Door codes recorded as passages | **0** | The webhook misread is a real bug but it never fired. `door_events` shows no `ALARM_FORCED_OPEN`/`ALARM_TIMEOUT` ever received, and one `RESTORED` (131588), whose missing `enterNum`/`exitNum` wrote nothing. | | 4 | Counting rows from the webhook | **0** | Confirms counting is 100% polling. The webhook itself works: 3,453 door transitions arrived through it. | Conclusion: no historical data needs correcting. Everything in this PR is **preventive**, and the design assumptions (1:1 groups, no per-crossing counting path) are confirmed against production.
gabogg changed title from WIP: fix(occupancy): camera-group schema, orphan guard and migration — phase 1 (#29) to fix(occupancy): camera-group schema, orphan guard and migration — phase 1 (#29) 2026-09-23 15:05:06 +00:00
gabogg commented 2026-09-23 16:45:32 +00:00
Author
Owner
Copy link

Code review

Reviewed commit ba57b9e6f715d9356257ef4fd1e34324e1e94cea against base 72fdcdd26dc2d99ae13644da349522b8a609cf5b using independent Standards and Spec review passes.

Standards

Spec

  • P1 — The registered-camera invariant remains bypassable. The specification requires “Every counting event's camera_index_code is a registered camera,” but the new guard protects polling only. I reproduced POST /api/occupancy/event returning 200 and persisting seven passages for an unregistered camera. Those events can reach telemetry and hourly aggregates before quarantine. Enforce registration at the shared write boundary and cover the direct endpoint.

  • P2 — Missing groups receive no persisted flag. Acceptance requires groups absent from group_list to “set their flag,” but this branch only warns and skips. I reproduced a grp-ghost count response producing no group record. Consequently, the designated operator endpoint cannot reveal the skipped group. Persist its unmapped/empty state before continuing.

Verification

223 tests passed, 1 skipped, with three aiosqlite thread warnings. Ruff passed. Both specification findings were reproduced with isolated SQLite and mocked hardware.

Standards: 1 finding, worst P3 (annotations). Spec: 2 findings, worst P1 (invariant bypass).

## Code review Reviewed commit `ba57b9e6f715d9356257ef4fd1e34324e1e94cea` against base `72fdcdd26dc2d99ae13644da349522b8a609cf5b` using independent Standards and Spec review passes. ## Standards - **P3 — Missing required type annotations.** [`get_camera_groups`](https://git.gaboggamer.online/gabogg/hikcentral/src/commit/ba57b9e6f715d9356257ef4fd1e34324e1e94cea/app/controllers/occupancy_controller.py#L86) lacks its return annotation; new [test fixtures and helpers](https://git.gaboggamer.online/gabogg/hikcentral/src/commit/ba57b9e6f715d9356257ef4fd1e34324e1e94cea/tests/test_counting_integrity.py#L23) also omit annotations. [Code standards §2.2](https://git.gaboggamer.online/gabogg/hikcentral/src/commit/ba57b9e6f715d9356257ef4fd1e34324e1e94cea/docs/standards/code-standards.md#L61) requires explicit parameter and return types. Ruff's configured rules do not enforce this requirement. ## Spec - **P1 — The registered-camera invariant remains bypassable.** The specification requires “Every counting event's `camera_index_code` is a registered camera,” but the [new guard](https://git.gaboggamer.online/gabogg/hikcentral/src/commit/ba57b9e6f715d9356257ef4fd1e34324e1e94cea/app/services/occupancy_service.py#L1954) protects polling only. I reproduced `POST /api/occupancy/event` returning **200** and persisting **seven passages for an unregistered camera**. Those events can reach telemetry and hourly aggregates before quarantine. Enforce registration at the shared write boundary and cover the direct endpoint. - **P2 — Missing groups receive no persisted flag.** Acceptance requires groups absent from `group_list` to “set their flag,” but [this branch](https://git.gaboggamer.online/gabogg/hikcentral/src/commit/ba57b9e6f715d9356257ef4fd1e34324e1e94cea/app/services/occupancy_service.py#L1945) only warns and skips. I reproduced a `grp-ghost` count response producing no group record. Consequently, the designated operator endpoint cannot reveal the skipped group. Persist its unmapped/empty state before continuing. ## Verification **223 tests passed, 1 skipped**, with three `aiosqlite` thread warnings. Ruff passed. Both specification findings were reproduced with isolated SQLite and mocked hardware. **Standards: 1 finding, worst P3 (annotations). Spec: 2 findings, worst P1 (invariant bypass).**
fix(occupancy): address PR #53 review — shared write guard, unlisted groups, annotations
All checks were successful
CI / lint-and-test (pull_request) Successful in 1m13s
Details
f4e38a831d 
P1: enforce the registered-camera invariant at the shared write boundary.
record_counting_event_async now raises UnregisteredCameraError for a
camera_index_code with no counting_cameras row, so every writer (sync,
simulator, the direct POST /api/occupancy/event endpoint) is covered, not
only the polling sync. The endpoint maps it to 422 CAMERA_NOT_REGISTERED.

P2: persist groups that HikCentral counts but does not list.
counting_camera_groups gains is_unlisted; the sync flags such a group
before skipping it, so GET /api/occupancy/camera-groups reveals it. The
flag clears when a camera sync lists the group.

P3: add the missing return and parameter annotations on the new endpoint,
exception and test fixtures/helpers (code standards section 2.2).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-09-23 17:03:12 +00:00
Author
Owner
Copy link

Review addressed — f4e38a8

P1: the invariant could be bypassed. Fixed at the shared write boundary. record_counting_event_async now looks up the camera and raises UnregisteredCameraError when there is no counting_cameras row. The guard sits on the one method every writer goes through (sync, simulator, POST /api/occupancy/event), not only on polling. The endpoint maps the error to 422 CAMERA_NOT_REGISTERED. New tests cover it at the service level and through the direct endpoint: an unregistered camera returns 422 and writes no row; a registered one returns 200.

  • Adjusted test_occupancy_api_endpoints_rbac: it posted an event for a camera it never registered, so it now registers the camera first. That test relied on the behaviour this PR forbids.

P2: missing groups had no persisted flag. Fixed. counting_camera_groups gains is_unlisted. When HikCentral returns counts for a group it does not list, the sync writes that flag (once per state change, not every poll) before skipping the group, so GET /api/occupancy/camera-groups shows it. A later camera sync that lists the group clears the flag and records its real name. The quarantine reason group_code covers these codes too. Test extended.

  • No ALTER TABLE for the new column: the table itself is introduced by this unmerged PR.

P3: annotations. Fixed. Added the return type on get_camera_groups, -> None on UnregisteredCameraError.__init__, and full parameter/return annotations on the test fixture, helpers and test functions (pytest.MonkeyPatch, pytest.LogCaptureFixture).

Verification: pytest 225 passed / 1 skipped, node --test 59/59, ruff check + ruff format --check clean, pre-commit hooks passed.

Out of scope, tracked in #64: POST /api/occupancy/event has no authentication dependency. Anyone who can reach the server can inject passages for any registered camera. P1 limits the damage to registered cameras; it does not close the hole. #64 also covers the unvalidated payload (negative count, the PASS direction, backdated timestamps) and recommends deleting the endpoint, since nothing calls it.

## Review addressed — `f4e38a8` **P1: the invariant could be bypassed. Fixed at the shared write boundary.** `record_counting_event_async` now looks up the camera and raises `UnregisteredCameraError` when there is no `counting_cameras` row. The guard sits on the one method every writer goes through (sync, simulator, `POST /api/occupancy/event`), not only on polling. The endpoint maps the error to **422 `CAMERA_NOT_REGISTERED`**. New tests cover it at the service level and through the direct endpoint: an unregistered camera returns 422 and writes no row; a registered one returns 200. - Adjusted `test_occupancy_api_endpoints_rbac`: it posted an event for a camera it never registered, so it now registers the camera first. That test relied on the behaviour this PR forbids. **P2: missing groups had no persisted flag. Fixed.** `counting_camera_groups` gains `is_unlisted`. When HikCentral returns counts for a group it does not list, the sync writes that flag (once per state change, not every poll) before skipping the group, so `GET /api/occupancy/camera-groups` shows it. A later camera sync that lists the group clears the flag and records its real name. The quarantine reason `group_code` covers these codes too. Test extended. - No `ALTER TABLE` for the new column: the table itself is introduced by this unmerged PR. **P3: annotations. Fixed.** Added the return type on `get_camera_groups`, `-> None` on `UnregisteredCameraError.__init__`, and full parameter/return annotations on the test fixture, helpers and test functions (`pytest.MonkeyPatch`, `pytest.LogCaptureFixture`). **Verification:** pytest **225 passed / 1 skipped**, `node --test` **59/59**, `ruff check` + `ruff format --check` clean, pre-commit hooks passed. **Out of scope, tracked in #64:** `POST /api/occupancy/event` has **no authentication dependency**. Anyone who can reach the server can inject passages for any registered camera. P1 limits the damage to registered cameras; it does not close the hole. #64 also covers the unvalidated payload (negative `count`, the `PASS` direction, backdated timestamps) and recommends deleting the endpoint, since nothing calls it.
gabogg commented 2026-09-23 17:19:15 +00:00
Author
Owner
Copy link

Code review — round 2

Reviewed f4e38a8 against master (72fdcdd) with two independent passes: Standards (docs/standards/code-standards.md, AGENTS.md, CONTEXT.md, plus a Fowler code-smell baseline) and Spec (this PR's description, #29, and the deviations declared in the comments above). The Spec pass ran the suite in a throwaway worktree: 225 passed, 1 skipped.

Standards

Layering, the error response shape, logging and the testing approach all follow the repo standards.

Documented standards

  • P3, hard rule (§2.2 "All function definitions… must include explicit type hints"), app/controllers/webhook_controller.py:18-22: this PR edits the signature of receive_event_webhook, but it still has no return type. It is the only annotation gap left.
  • P2, judgement (§2.3 "Avoid passing untyped, raw dictionaries between service layers when structured schemas are available"): group rows travel between service and repository as dict[str, Any] (occupancy_service.py:429-445, occupancy_repository.py:634), although CameraGroupItem exists. The same keys always travel together, which also makes them a data clump.
  • P3, occupancy_controller.py:364: status_code=422 is a bare literal; the §3.1 example uses status.HTTP_… constants.

Code smells (judgement calls)

  • P3, possible Duplicated Code, occupancy_repository.py:665 and :701: the two INSERT … ON CONFLICT upserts on counting_camera_groups have the same shape, and the int-to-bool row mapping is written twice.
  • P3, possible Mysterious Name, occupancy_service.py:1955-1957: _skipped_group_warnings, a set named for warnings, also decides whether to write to the database.
  • P3, possible Primitive Obsession: member cameras travel as (c_code, c_name) tuples; quarantine reasons are bare strings in both the SQL and the summary.
  • P3, glossary: "counting event quarantine" and the group states (is_empty, is_unlisted, is_multi_camera) are not defined in CONTEXT.md.
  • P3, database.py:282-285: the new migration uses except Exception: pass (copying the file's existing pattern); sqlite3.OperationalError would be narrower.

Spec

Previous round: P1 (invariant bypass) fixed: the sync, the simulator and POST /api/occupancy/event all write through the guarded record_counting_event_async. P2 (unlisted groups) fixed only in the straightforward case, see below.

Implemented but wrong

  • P2, reproduced. Spec: "groups missing from group_list emit zero events, set their flag, and log one warning." mark_group_unlisted_async runs only when the group code is not already in the warn-once set, but groups skipped for other reasons (empty, unregistered member) land in the same set. A group first skipped as empty and later dropped from the list never gets is_unlisted, and gets no new warning. Reproduced: is_unlisted: False, only the "no member cameras" warning logged.
  • P3, not reproduced. Quarantine labels a row group_code by looking in counting_camera_groups. If the startup camera sync fails on an upgraded DB, that table is empty and legacy group-code rows are labelled unknown. Only the label is affected; production has 0 orphans.

Missing or partial

  • P3, not reproduced. Spec: "Integrity check runs at startup and periodically." The startup call (main.py:42) shares a try with door_manager.sync_doors_async(). If the door sync raises, the check is skipped, and the monitor schedules its first run an hour later (monitor_service.py:93).

Scope creep: none beyond the deviations declared in the implementation notes; the code matches each one.

Standards: 8 findings, worst P2 (raw dicts between service and repository). Spec: 3 findings, worst P2 (the unlisted flag shares the warn-once set). All of them are being addressed in a follow-up commit.

## Code review — round 2 Reviewed `f4e38a8` against `master` (`72fdcdd`) with two independent passes: **Standards** (`docs/standards/code-standards.md`, `AGENTS.md`, `CONTEXT.md`, plus a Fowler code-smell baseline) and **Spec** (this PR's description, #29, and the deviations declared in the comments above). The Spec pass ran the suite in a throwaway worktree: **225 passed, 1 skipped**. ## Standards Layering, the error response shape, logging and the testing approach all follow the repo standards. **Documented standards** - **P3, hard rule (§2.2 "All function definitions… must include explicit type hints")**, `app/controllers/webhook_controller.py:18-22`: this PR edits the signature of `receive_event_webhook`, but it still has no return type. It is the only annotation gap left. - **P2, judgement (§2.3 "Avoid passing untyped, raw dictionaries between service layers when structured schemas are available")**: group rows travel between service and repository as `dict[str, Any]` (`occupancy_service.py:429-445`, `occupancy_repository.py:634`), although `CameraGroupItem` exists. The same keys always travel together, which also makes them a data clump. - **P3**, `occupancy_controller.py:364`: `status_code=422` is a bare literal; the §3.1 example uses `status.HTTP_…` constants. **Code smells (judgement calls)** - **P3, possible Duplicated Code**, `occupancy_repository.py:665` and `:701`: the two `INSERT … ON CONFLICT` upserts on `counting_camera_groups` have the same shape, and the int-to-bool row mapping is written twice. - **P3, possible Mysterious Name**, `occupancy_service.py:1955-1957`: `_skipped_group_warnings`, a set named for warnings, also decides whether to write to the database. - **P3, possible Primitive Obsession**: member cameras travel as `(c_code, c_name)` tuples; quarantine reasons are bare strings in both the SQL and the summary. - **P3, glossary**: "counting event quarantine" and the group states (`is_empty`, `is_unlisted`, `is_multi_camera`) are not defined in `CONTEXT.md`. - **P3**, `database.py:282-285`: the new migration uses `except Exception: pass` (copying the file's existing pattern); `sqlite3.OperationalError` would be narrower. ## Spec **Previous round:** **P1** (invariant bypass) **fixed**: the sync, the simulator and `POST /api/occupancy/event` all write through the guarded `record_counting_event_async`. **P2** (unlisted groups) **fixed only in the straightforward case**, see below. **Implemented but wrong** - **P2, reproduced.** Spec: *"groups missing from `group_list` emit zero events, set their flag, and log one warning."* `mark_group_unlisted_async` runs only when the group code is not already in the warn-once set, but groups skipped for **other** reasons (empty, unregistered member) land in the same set. A group first skipped as empty and later dropped from the list never gets `is_unlisted`, and gets no new warning. Reproduced: `is_unlisted: False`, only the "no member cameras" warning logged. - **P3, not reproduced.** Quarantine labels a row `group_code` by looking in `counting_camera_groups`. If the startup camera sync fails on an upgraded DB, that table is empty and legacy group-code rows are labelled `unknown`. Only the label is affected; production has 0 orphans. **Missing or partial** - **P3, not reproduced.** Spec: *"Integrity check runs at startup and periodically."* The startup call (`main.py:42`) shares a `try` with `door_manager.sync_doors_async()`. If the door sync raises, the check is skipped, and the monitor schedules its first run an hour later (`monitor_service.py:93`). **Scope creep:** none beyond the deviations declared in the implementation notes; the code matches each one. --- **Standards: 8 findings, worst P2** (raw dicts between service and repository). **Spec: 3 findings, worst P2** (the unlisted flag shares the warn-once set). All of them are being addressed in a follow-up commit.
fix(occupancy): address PR #53 review round 2
All checks were successful
CI / lint-and-test (pull_request) Successful in 1m10s
Details
f899891c1d 
Spec:
- The unlisted flag no longer shares the warn-once set with other skip
  reasons. The sync tracks the current skip reason per group
  (GroupSkipReason) and acts on changes only: a group first skipped as
  empty and later dropped from HikCentral's list is now flagged
  is_unlisted and warned about, keeping its known name.
- Quarantine waits until both cameras and camera groups are registered,
  so a legacy group-code row is never mislabelled `unknown`.
- Door and occupancy startup run in separate try blocks, so a failed door
  sync can no longer skip the startup counting integrity check.

Standards:
- Group rows travel as typed schemas (CameraGroupRegistration,
  CameraGroupRegistrationResult, CameraGroupItem) instead of raw dicts.
- One write path and one row mapper for counting_camera_groups.
- Quarantine reasons are a QuarantineReason enum with QuarantineTally
  counts; group members are a GroupMember NamedTuple.
- webhook return type, status.HTTP_422_UNPROCESSABLE_CONTENT, narrower
  sqlite3.OperationalError on the new migration, aiosqlite types.
- CONTEXT.md: Camera Group States and Quarantined Counting Event.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-09-23 17:32:33 +00:00
Author
Owner
Copy link

Review round 2 addressed — f899891

All 11 findings from the round-2 review above are fixed.

Spec

  • P2, unlisted flag shared the warn-once set: fixed. The sync now tracks the current skip reason per group (GroupSkipReason: UNLISTED, EMPTY, UNREGISTERED_MEMBER) and acts only when the reason changes: it warns once per new reason, and persists is_unlisted when a group enters UNLISTED. A group first skipped as empty and later dropped from HikCentral's list now gets the flag and a second warning, and keeps its known name. Regression test: test_group_turning_unlisted_after_another_skip_is_flagged, which covers the scenario the reviewer reproduced.
  • P3, quarantine could mislabel group-code rows: fixed. Quarantine now waits until both cameras and camera groups are registered, and returns None before that. A legacy group-code row can no longer be labelled unknown. Test: test_quarantine_skipped_until_camera_groups_registered.
  • P3, a door failure could skip the startup check: fixed. Door startup and occupancy startup are separate try blocks in main.py, so a failed door sync no longer skips the counting integrity check. Test: test_startup_integrity_check_runs_when_door_sync_fails (runs the real lifespan with the door sync raising).

Standards

  • P2, raw dicts between layers: fixed. Group rows travel as schemas: CameraGroupRegistration in, CameraGroupRegistrationResult (group + previous) out, and get_camera_groups_async returns list[CameraGroupItem], which the endpoint returns directly.
  • P3, duplicated upsert and bool mapping: fixed. counting_camera_groups now has one write path (_save_camera_group) and one row mapper (_row_to_camera_group); mark_group_unlisted_async reads the row, sets the flag and saves it through the same path.
  • P3, set named for warnings guarding a DB write: fixed by the GroupSkipReason change above; the set is gone.
  • P3, primitives: fixed. QuarantineReason enum (values bound as SQL parameters, not literals) with QuarantineTally counts; group members are a GroupMember NamedTuple.
  • P3, webhook return type: fixed (-> dict[str, str]). The repository helpers also take aiosqlite.Connection / aiosqlite.Row instead of Any.
  • P3, bare 422: fixed. Uses status.HTTP_422_UNPROCESSABLE_CONTENT; the _ENTITY name is deprecated in the installed Starlette.
  • P3, broad except: fixed on the new migration (sqlite3.OperationalError). The file's pre-existing except Exception blocks were left alone.
  • P3, glossary: fixed. CONTEXT.md §3 gains Camera Group States (empty / unlisted / multi-camera) and Quarantined Counting Event. They are added at the end of §3, away from the lines #54 edits, to avoid a merge conflict. The pre-commit end-of-file hook also removed two trailing blank lines from the file.

Verification

pytest 228 passed / 1 skipped (3 new tests), node --test 59/59, ruff check + ruff format --check clean, pre-commit hooks passed.

## Review round 2 addressed — `f899891` All 11 findings from the round-2 review above are fixed. ### Spec - **P2, unlisted flag shared the warn-once set: fixed.** The sync now tracks the **current skip reason per group** (`GroupSkipReason`: `UNLISTED`, `EMPTY`, `UNREGISTERED_MEMBER`) and acts only when the reason changes: it warns once per new reason, and persists `is_unlisted` when a group enters `UNLISTED`. A group first skipped as empty and later dropped from HikCentral's list now gets the flag and a second warning, and keeps its known name. Regression test: `test_group_turning_unlisted_after_another_skip_is_flagged`, which covers the scenario the reviewer reproduced. - **P3, quarantine could mislabel group-code rows: fixed.** Quarantine now waits until **both** cameras and camera groups are registered, and returns `None` before that. A legacy group-code row can no longer be labelled `unknown`. Test: `test_quarantine_skipped_until_camera_groups_registered`. - **P3, a door failure could skip the startup check: fixed.** Door startup and occupancy startup are separate `try` blocks in `main.py`, so a failed door sync no longer skips the counting integrity check. Test: `test_startup_integrity_check_runs_when_door_sync_fails` (runs the real lifespan with the door sync raising). ### Standards - **P2, raw dicts between layers: fixed.** Group rows travel as schemas: `CameraGroupRegistration` in, `CameraGroupRegistrationResult` (`group` + `previous`) out, and `get_camera_groups_async` returns `list[CameraGroupItem]`, which the endpoint returns directly. - **P3, duplicated upsert and bool mapping: fixed.** `counting_camera_groups` now has one write path (`_save_camera_group`) and one row mapper (`_row_to_camera_group`); `mark_group_unlisted_async` reads the row, sets the flag and saves it through the same path. - **P3, set named for warnings guarding a DB write: fixed** by the `GroupSkipReason` change above; the set is gone. - **P3, primitives: fixed.** `QuarantineReason` enum (values bound as SQL parameters, not literals) with `QuarantineTally` counts; group members are a `GroupMember` NamedTuple. - **P3, webhook return type: fixed** (`-> dict[str, str]`). The repository helpers also take `aiosqlite.Connection` / `aiosqlite.Row` instead of `Any`. - **P3, bare 422: fixed.** Uses `status.HTTP_422_UNPROCESSABLE_CONTENT`; the `_ENTITY` name is deprecated in the installed Starlette. - **P3, broad except: fixed** on the new migration (`sqlite3.OperationalError`). The file's pre-existing `except Exception` blocks were left alone. - **P3, glossary: fixed.** `CONTEXT.md` §3 gains **Camera Group States** (empty / unlisted / multi-camera) and **Quarantined Counting Event**. They are added at the end of §3, away from the lines #54 edits, to avoid a merge conflict. The pre-commit end-of-file hook also removed two trailing blank lines from the file. ### Verification pytest **228 passed / 1 skipped** (3 new tests), `node --test` **59/59**, `ruff check` + `ruff format --check` clean, pre-commit hooks passed.
gabogg commented 2026-09-23 17:41:12 +00:00
Author
Owner
Copy link

Code review — round 3 (pre-merge)

Reviewed f899891 against master (72fdcdd) with two independent passes: Standards (docs/standards/code-standards.md, AGENTS.md, CONTEXT.md, plus a Fowler code-smell baseline) and Spec (this PR's description, #29, and the amendments and review responses above).

Pre-flight: mergeable into master with no conflicts · CI lint-and-test green on f899891 · pytest 228 passed / 1 skipped and node --test 59/59, re-run by the Spec pass in a throwaway worktree.

Verdict: not ready to merge yet. Standards is clear. Spec has one merge blocker, a regression introduced by the round-2 fix.

Standards

All round 1–2 fixes hold. No merge blockers, and no documented rule broken outright.

  • P2, raw dict where a schema exists (§2.3). The new get_camera_async returns dict[str, Any] | None (occupancy_repository.py:631, used at occupancy_service.py:616) although CountingCameraItem exists. cam_payload also gains an untyped resource_group_code key (occupancy_service.py:477).
  • P3, in-memory state still decides a DB write. _group_skip_reasons controls whether mark_group_unlisted_async runs (occupancy_service.py:574-578). Camera sync clears the DB flag without touching the dict, so the two can drift.
  • P3, possible Data Clump. group_cam_map[g_code] = {"name", "in_cams", "out_cams"} (:1933) is still a raw dict (the shape predates this PR; the PR edits it).
  • P3. GroupMember / GroupSkipReason live in the service while QuarantineReason lives in app/schemas/; GroupSkipReason values are log sentences.
  • P3, possible Mysterious Name. QuarantineTally.total means passages; passages would say so, and the class has no docstring.
  • P3. quarantine_orphan_counting_events_async returns None (skipped), {} (nothing moved) or a summary. It is documented, but easy to misread.
  • P3. The controller uses HTTP_422_UNPROCESSABLE_CONTENT while main.py:83/100 still uses _ENTITY. Production runs Starlette 1.6.0 (checked read-only), which has _CONTENT, so there is no deploy risk.

Spec

All three round-2 fixes verified by behaviour. Nothing from the acceptance criteria is missing; no scope creep.

Implemented but wrong

  • P2, reproduced, merge blocker. Spec: "Integrity check… Startup and periodic… Log loudly when non-empty." Since the round-2 fix, the check returns None whenever counting_camera_groups is empty (occupancy_repository.py:768). A deployment whose cameras came from the fallback camera list (used when resourceGroupList is unavailable) never registers a group, so the check is permanently off, logs only at INFO (occupancy_service.py:594), and orphans stay put. Reproduced with a legacy grp-legacy row, 1 camera, 0 groups. Production has groups and is not affected today.
  • P3, reproduced. _skip_group_async records the reason in memory (occupancy_service.py:576) before mark_group_unlisted_async writes. If that write fails once, later polls see the same reason and never retry, so the group never appears in GET /camera-groups.
  • P3, reproduced. A group that is unlisted and then disappears from count_list altogether stays is_unlisted=True forever, and its stale _group_skip_reasons entry suppresses the warning if it returns with the same reason.

Note (P3, not reproduced): if enforce_counting_integrity_async raises at startup (main.py:47), sync_initial_counts and reconcile_and_quarantine_historical_anomalies_async are skipped because they share its try.

Standards: 7 findings, worst P2 (get_camera_async raw dict). Spec: 3 findings + 1 note, worst P2 (integrity check permanently off without camera groups, merge blocker).

Plan: fix the Spec P2 in this PR before merge; move every other finding to a follow-up issue.

## Code review — round 3 (pre-merge) Reviewed `f899891` against `master` (`72fdcdd`) with two independent passes: **Standards** (`docs/standards/code-standards.md`, `AGENTS.md`, `CONTEXT.md`, plus a Fowler code-smell baseline) and **Spec** (this PR's description, #29, and the amendments and review responses above). **Pre-flight:** mergeable into `master` with no conflicts · CI `lint-and-test` green on `f899891` · pytest **228 passed / 1 skipped** and `node --test` **59/59**, re-run by the Spec pass in a throwaway worktree. **Verdict: not ready to merge yet.** Standards is clear. Spec has one merge blocker, a regression introduced by the round-2 fix. ## Standards All round 1–2 fixes hold. No merge blockers, and no documented rule broken outright. - **P2, raw dict where a schema exists (§2.3).** The new `get_camera_async` returns `dict[str, Any] | None` (`occupancy_repository.py:631`, used at `occupancy_service.py:616`) although `CountingCameraItem` exists. `cam_payload` also gains an untyped `resource_group_code` key (`occupancy_service.py:477`). - **P3, in-memory state still decides a DB write.** `_group_skip_reasons` controls whether `mark_group_unlisted_async` runs (`occupancy_service.py:574-578`). Camera sync clears the DB flag without touching the dict, so the two can drift. - **P3, possible Data Clump.** `group_cam_map[g_code] = {"name", "in_cams", "out_cams"}` (`:1933`) is still a raw dict (the shape predates this PR; the PR edits it). - **P3.** `GroupMember` / `GroupSkipReason` live in the service while `QuarantineReason` lives in `app/schemas/`; `GroupSkipReason` values are log sentences. - **P3, possible Mysterious Name.** `QuarantineTally.total` means passages; `passages` would say so, and the class has no docstring. - **P3.** `quarantine_orphan_counting_events_async` returns `None` (skipped), `{}` (nothing moved) or a summary. It is documented, but easy to misread. - **P3.** The controller uses `HTTP_422_UNPROCESSABLE_CONTENT` while `main.py:83/100` still uses `_ENTITY`. Production runs Starlette 1.6.0 (checked read-only), which has `_CONTENT`, so there is no deploy risk. ## Spec All three round-2 fixes verified by behaviour. Nothing from the acceptance criteria is missing; no scope creep. **Implemented but wrong** - **P2, reproduced, merge blocker.** Spec: *"Integrity check… Startup and periodic… Log loudly when non-empty."* Since the round-2 fix, the check returns `None` whenever `counting_camera_groups` is empty (`occupancy_repository.py:768`). A deployment whose cameras came from the fallback camera list (used when `resourceGroupList` is unavailable) never registers a group, so the check is **permanently off**, logs only at INFO (`occupancy_service.py:594`), and orphans stay put. Reproduced with a legacy `grp-legacy` row, 1 camera, 0 groups. Production has groups and is not affected today. - **P3, reproduced.** `_skip_group_async` records the reason in memory (`occupancy_service.py:576`) **before** `mark_group_unlisted_async` writes. If that write fails once, later polls see the same reason and never retry, so the group never appears in `GET /camera-groups`. - **P3, reproduced.** A group that is unlisted and then disappears from `count_list` altogether stays `is_unlisted=True` forever, and its stale `_group_skip_reasons` entry suppresses the warning if it returns with the same reason. **Note (P3, not reproduced):** if `enforce_counting_integrity_async` raises at startup (`main.py:47`), `sync_initial_counts` and `reconcile_and_quarantine_historical_anomalies_async` are skipped because they share its `try`. --- **Standards: 7 findings, worst P2** (`get_camera_async` raw dict). **Spec: 3 findings + 1 note, worst P2** (integrity check permanently off without camera groups, merge blocker). **Plan:** fix the Spec P2 in this PR before merge; move every other finding to a follow-up issue.
fix(occupancy): keep the counting integrity check on without camera groups
All checks were successful
CI / lint-and-test (pull_request) Successful in 1m12s
Details
039ae9100a 
Round-3 review (PR #53), Spec P2: since the round-2 fix, the integrity
check returned None whenever counting_camera_groups was empty. A
deployment whose cameras come from the camera-list fallback never
registers a group, so the check was permanently off, logged only at INFO,
and orphans stayed put.

The check now returns a typed CountingIntegrityReport instead of
None / {} / a summary:
- SKIPPED_NO_CAMERAS: empty registry, every event would look orphaned.
- DETECTED_ONLY: cameras but no groups. Orphans are counted and logged at
  ERROR, but left in place, because a group-code row cannot be told apart
  from an unknown one yet. A later run with groups registered quarantines
  them with the correct reason.
- CHECKED: orphans quarantined per reason, as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-09-23 17:48:44 +00:00
Author
Owner
Copy link

Review round 3 addressed — 039ae91

Merge blocker (Spec P2) fixed. The integrity check no longer switches itself off when no camera group is registered. It now returns a typed CountingIntegrityReport instead of None / {} / a summary:

Status When What happens
SKIPPED_NO_CAMERAS no cameras registered skipped: every event would look orphaned
DETECTED_ONLY cameras but no camera groups (e.g. the camera-list fallback) orphans counted and logged at ERROR, but left in place, because a group-code row can't be labelled yet
CHECKED cameras and groups orphans quarantined per reason, as before

A deployment on the fallback now gets a loud error every hour instead of silence. Once groups register, the next run quarantines the same rows with the correct reason, so round 2's mislabelling fix still holds. This also resolves the Standards P3 about the return value having three meanings.

New tests: test_orphans_detected_and_logged_when_no_camera_groups (the scenario the reviewer reproduced) and test_orphans_quarantined_with_correct_reason_once_groups_register.

Everything else (Standards P2 on get_camera_async, the remaining Standards P3s, and the three Spec P3s) is non-blocking and moved to #65.

Verification: pytest 229 passed / 1 skipped, node --test 59/59, ruff clean, pre-commit hooks passed.

## Review round 3 addressed — `039ae91` **Merge blocker (Spec P2) fixed.** The integrity check no longer switches itself off when no camera group is registered. It now returns a typed `CountingIntegrityReport` instead of `None` / `{}` / a summary: | Status | When | What happens | |---|---|---| | `SKIPPED_NO_CAMERAS` | no cameras registered | skipped: every event would look orphaned | | `DETECTED_ONLY` | cameras but no camera groups (e.g. the camera-list fallback) | orphans **counted and logged at ERROR**, but left in place, because a group-code row can't be labelled yet | | `CHECKED` | cameras and groups | orphans quarantined per reason, as before | A deployment on the fallback now gets a loud error every hour instead of silence. Once groups register, the next run quarantines the same rows with the **correct** reason, so round 2's mislabelling fix still holds. This also resolves the Standards P3 about the return value having three meanings. New tests: `test_orphans_detected_and_logged_when_no_camera_groups` (the scenario the reviewer reproduced) and `test_orphans_quarantined_with_correct_reason_once_groups_register`. **Everything else** (Standards P2 on `get_camera_async`, the remaining Standards P3s, and the three Spec P3s) is non-blocking and moved to **#65**. **Verification:** pytest **229 passed / 1 skipped**, `node --test` **59/59**, ruff clean, pre-commit hooks passed.
gabogg merged commit 1ed0f459bf into master 2026-09-23 17:51:11 +00:00
gabogg deleted branch fix/counting-events-camera-integrity 2026-09-23 17:51:12 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
blocked
Waits on the issues listed in its "Blocked by:" line before implementation

  
bug
Category: something is broken; behaviour differs from the spec or docs

  
enhancement
Category: new feature or improvement, including refactors, chores and docs

  
high-priority
Address before ordinary-priority work; independent of triage readiness and blockers

  
low-priority
Address after ordinary-priority work; independent of triage readiness and blockers

  
needs-info
Waiting on reporter for more information

  
needs-triage
Maintainer needs to evaluate this issue

  
ready-for-agent
Fully specified, ready for an AFK agent

  
ready-for-human
Requires human implementation

  
referenced
Previously closed issue reopened or referenced by an open issue; visible history marker

  
research
Needs investigation (internet or local resources) before it can be triaged down; used alongside a category and a triage role

  
wontfix
Will not be actioned

No labels
blocked
bug
enhancement
high-priority
low-priority
needs-info
needs-triage
ready-for-agent
ready-for-human
referenced
research
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gabogg/hikcentral!53
No description provided.