feat(telemetry): broadcast door state transitions as their own message (#55) #94

Merged

gabogg merged 5 commits from feat/telemetry-door-transition-broadcast into master 2026-09-25 12:19:43 +00:00

Conversation 2 Commits 5 Files changed 8 +686 -152

gabogg commented 2026-09-25 01:27:35 +00:00

Owner

Copy link

Closes #55

Problem

Door state transitions were written to door_hardware_state_transitions but never broadcast. A client could only infer them by diffing successive doors_update overviews, which misses any change that happens between two polls.

Approach

This follows the 2026-09-23 triage resolution on #55.

Message. A new DoorStateTransition DTO in app/schemas/models.py has these fields: id, door_index_code, door_name, previous_state, new_state, state_key, trigger_source, timestamp_epoch, exclude_from_rankings and exclusion_source. It covers physical, commanded (remain open or closed) and offline/recovery changes. It carries no details_json, person names or card numbers; those stay in the access-cycle messages. The message is {"type": "door_transitions", "transitions": [...], "server_time", "facility_utc_offset_minutes"}.

Emission (server).

• All four persistence paths go through DoorManager._record_transitions_async / _record_transitions_sync: async polling, sync polling, webhook and reconciliation. Each one persists first and then broadcasts one message per persisted batch, containing each new transition once.
• If persistence raises, nothing is broadcast. A broadcast failure is logged and doesn't break the poll.
• The sync paths schedule the broadcast on the running loop; with no loop (CLI), the transition is persisted and there is nobody to broadcast to.
• Repositories stay persistence-only. record_hardware_transitions_batch_async / _sync now return the new row ids in input order, and the four copies of the INSERT statement became one _INSERT_TRANSITION_SQL.
• Excluded doors are included, with their exclusion metadata, so the hardware inventory stays current.
• Delivery is best effort, as the triage decided: an emission attempt is not an acknowledgment, and transitions missed while disconnected are not replayed. The doors_update sent on connect still restores current state.

Ingestion (client). TelemetryEngine handles door_transitions:

• each transition moves one door to its new state (open, remain open, closed, offline) without waiting for an overview;
• a batch is applied oldest first, and an older transition arriving late doesn't undo a newer state;
• ids are de-duplicated;
• a transition for a door the engine doesn't know yet is buffered, not invented;
• excluded doors are updated and stay excluded, so rankings and the activity stream keep filtering them as before;
• the snapshot exposes the latest 50 transitions as doorTransitions (DOOR_TRANSITION_BUFFER_SIZE);
• overview snapshots remain the source for initialization, reconnect and derived metadata.

Docs. docs/api/README.md now lists the /ws/realtime message types.

Verification

• New tests/test_door_transition_broadcast.py (9 tests, real SQLite, HikCentral stubbed):
  • the webhook transition is broadcast once, with the id of the stored row and no extra fields;
  • polling: a commanded REMAIN_OPEN, an offline change and its recovery;
  • an excluded door carries its exclusion;
  • the reconciliation path;
  • the sync polling path;
  • failed persistence broadcasts nothing;
  • one poll with two changed doors gives one message with two distinct ids;
  • a new connection still receives the initial doors_update.
• 9 new engine tests in tests/frontend/test_telemetry_engine.test.js: state change without an overview, remain-open and offline, a late transition, batch ordering, repeated ids, excluded doors, unknown doors, the buffer limit, and resync by the next overview.
• pytest: 291 passed, 1 skipped. node --test tests/frontend/*.test.js: 76 passed. ruff is clean, scripts/check_docs.py passes, and pre-commit passed.

Notes for review

• Overlap with the other telemetry PRs:

  • #93 (#46) also edits the webhook handler in door_service.py.
  • #91 (#67) edits JSDoc in telemetry_engine.js.

  The hunks are in different places, but whichever PR merges second may need a small rebase.

• Displaying transitions: nothing in the UI renders doorTransitions yet. The engine keeps door state current from them, and a transition log panel can read the buffer later.

🤖 Generated with Claude Code

Checklist

• Persisted transition batches broadcast once on an active event loop.
• Background broadcast tasks stay referenced until complete.
• Backend and frontend transition tests, Ruff, and full pytest pass.

Review follow-up

Broadcast tasks are retained until complete; first-seen door exclusions are read from persistence; unused single-row transition writers were removed. The telemetry engine updates door state immediately, while the dashboard grid still renders on the next doors_update overview. The pre-existing synchronous database work in the async webhook route is tracked by #95.

Closes #55 ## Problem Door state transitions were written to `door_hardware_state_transitions` but never broadcast. A client could only infer them by diffing successive `doors_update` overviews, which misses any change that happens between two polls. ## Approach This follows the 2026-09-23 triage resolution on #55. **Message.** A new `DoorStateTransition` DTO in `app/schemas/models.py` has these fields: `id`, `door_index_code`, `door_name`, `previous_state`, `new_state`, `state_key`, `trigger_source`, `timestamp_epoch`, `exclude_from_rankings` and `exclusion_source`. It covers physical, commanded (remain open or closed) and offline/recovery changes. It carries no `details_json`, person names or card numbers; those stay in the access-cycle messages. The message is `{"type": "door_transitions", "transitions": [...], "server_time", "facility_utc_offset_minutes"}`. **Emission (server).** - All four persistence paths go through `DoorManager._record_transitions_async` / `_record_transitions_sync`: async polling, sync polling, webhook and reconciliation. Each one persists first and then broadcasts **one message per persisted batch**, containing each new transition once. - If persistence raises, nothing is broadcast. A broadcast failure is logged and doesn't break the poll. - The sync paths schedule the broadcast on the running loop; with no loop (CLI), the transition is persisted and there is nobody to broadcast to. - Repositories stay persistence-only. `record_hardware_transitions_batch_async` / `_sync` now return the new row ids in input order, and the four copies of the INSERT statement became one `_INSERT_TRANSITION_SQL`. - Excluded doors are included, with their exclusion metadata, so the hardware inventory stays current. - Delivery is best effort, as the triage decided: an emission attempt is not an acknowledgment, and transitions missed while disconnected are not replayed. The `doors_update` sent on connect still restores current state. **Ingestion (client).** `TelemetryEngine` handles `door_transitions`: - each transition moves one door to its new state (open, remain open, closed, offline) without waiting for an overview; - a batch is applied oldest first, and an older transition arriving late doesn't undo a newer state; - ids are de-duplicated; - a transition for a door the engine doesn't know yet is buffered, not invented; - excluded doors are updated and stay excluded, so rankings and the activity stream keep filtering them as before; - the snapshot exposes the latest 50 transitions as `doorTransitions` (`DOOR_TRANSITION_BUFFER_SIZE`); - overview snapshots remain the source for initialization, reconnect and derived metadata. **Docs.** `docs/api/README.md` now lists the `/ws/realtime` message types. ## Verification - New `tests/test_door_transition_broadcast.py` (9 tests, real SQLite, HikCentral stubbed): - the webhook transition is broadcast once, with the id of the stored row and no extra fields; - polling: a commanded `REMAIN_OPEN`, an offline change and its recovery; - an excluded door carries its exclusion; - the reconciliation path; - the sync polling path; - failed persistence broadcasts nothing; - one poll with two changed doors gives one message with two distinct ids; - a new connection still receives the initial `doors_update`. - 9 new engine tests in `tests/frontend/test_telemetry_engine.test.js`: state change without an overview, remain-open and offline, a late transition, batch ordering, repeated ids, excluded doors, unknown doors, the buffer limit, and resync by the next overview. - `pytest`: **291 passed, 1 skipped**. `node --test tests/frontend/*.test.js`: **76 passed**. `ruff` is clean, `scripts/check_docs.py` passes, and pre-commit passed. ## Notes for review - **Overlap with the other telemetry PRs:** - #93 (#46) also edits the webhook handler in `door_service.py`. - #91 (#67) edits JSDoc in `telemetry_engine.js`. The hunks are in different places, but whichever PR merges second may need a small rebase. - **Displaying transitions:** nothing in the UI renders `doorTransitions` yet. The engine keeps door state current from them, and a transition log panel can read the buffer later. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Checklist - [x] Persisted transition batches broadcast once on an active event loop. - [x] Background broadcast tasks stay referenced until complete. - [x] Backend and frontend transition tests, Ruff, and full pytest pass. ## Review follow-up Broadcast tasks are retained until complete; first-seen door exclusions are read from persistence; unused single-row transition writers were removed. The telemetry engine updates door state immediately, while the dashboard grid still renders on the next `doors_update` overview. The pre-existing synchronous database work in the async webhook route is tracked by #95.

gabogg added 1 commit 2026-09-25 01:27:36 +00:00

feat(telemetry): broadcast door state transitions as their own message (#55) ... 54cc6a8de5

Door state transitions were persisted but never broadcast; clients had to
diff successive overviews and missed changes between two polls.

- New DoorStateTransition DTO: transition id, door, previous/new state,
  state key, source, timestamp and exclusion metadata. No audit details,
  person names or card numbers.
- All four persistence paths (async polling, sync polling, webhook,
  reconciliation) go through DoorManager._record_transitions_async/_sync,
  which persist and then broadcast one `door_transitions` message per
  batch. Repositories only persist; the batch methods now return the new
  row ids.
- TelemetryEngine applies `door_transitions`: each transition moves one
  door's state (oldest first, late ones ignored, ids de-duplicated) and the
  snapshot exposes the latest 50 as `doorTransitions`. Overview snapshots
  remain the source for initialization and reconnect.
- Document the /ws/realtime message types.

Closes #55

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

gabogg commented 2026-09-25 08:39:20 +00:00

Author

Owner

Copy link

Code review — pass 1 (two-axis)

Reviewed the PR's own diff (git diff <base>...<head>); Standards and Spec ran as independent passes and are reported separately, not reranked. Severity: P1 must fix · P2 fix before merge · P3 minor. Per review policy, pass 1 findings are all to be fixed on the branch.

Standards

No P1 bugs. Layering holds: SQL stays in the repository (_INSERT_TRANSITION_SQL also consolidates four copies), the DTO lives in app/schemas/, services do the broadcasting, and each new path has a test.

app/services/door_service.py

• P2, async hygiene (judgement call). In _record_transitions_sync (~L1397-1405), loop.create_task(self._broadcast_transitions(events)) discards the task handle. The loop holds only a weak reference (asyncio docs), so the task can be garbage-collected mid-run. The webhook path goes through here (called from the async /webhook route). Keep tasks in a set, or reuse the done-callback pattern.
• P3, possible Duplicated Code. That block re-implements _schedule_broadcast (~L1450): get_running_loop() / RuntimeError / create_task, minus the done-callback. Extract one _spawn(coro) helper. The now-redundant local import asyncio in _schedule_broadcast (~L1453) can go, since this PR adds it at module top.
• P3, silent drop. The except RuntimeError comment says "No event loop (CLI / sync callers): persisted, nothing to broadcast to". True only for CLI callers; a future caller on a worker thread (to_thread, threadpool def endpoint) would silently lose broadcasts while clients are connected. At least log at debug.
• P3, soft breach of code-standards.md §2.3 ("Avoid passing untyped, raw dictionaries between service layers when structured schemas are available"). The webhook hunk (~L858-880) builds a new 11-key dict[str, Any], and _transition_events re-reads it key by key. Also possible Data Clumps / Primitive Obsession: the same transition fields travel as a dict through four paths. A typed input record (a DoorStateTransition-like model without id) would remove the str()/int() coercions.
• P3, pre-existing. The async /webhook route still does blocking sync SQLite writes via handle_webhook_event, breaching §2.4 "Never call blocking synchronous functions within async routes". This PR adds one more sync write there. Worth an issue rather than a fix here.
• P3. door.get("exclusionSource") or door.get("exclusion_source") (~L1425) is a dual-key fallback; pick one canonical key.

app/db/door_repository.py

• P3. cursor.lastrowid is int | None, but methods now declare -> list[int] (~L664-689). Add an assert or cast.

app/static/js/src/telemetry/telemetry_engine.js

• P3, possible Repeated Switches. _ingestDoorTransitions (~L641-667) adds a third copy of the door-state → open/closed/offline flag logic (others in _ingestDoors and the snapshot builder).
• P3. Updates stateKey but leaves stateLabel stale (e.g. "🟢 Asegurada" on a now-open door) until the next overview. Nothing renders stateLabel today, so latent.

PR description

• P3. docs/standards/git-and-workflow.md §2.2 requires a Checklist section; none present.

Spec

Verdict: meets every acceptance criterion in the 2026-09-23 triage resolution. No P1 or P2 findings; the P3s are hardening and test gaps.

(a) Missing or partial

• P3 tests/test_door_transition_broadcast.py:222 — "failed persistence, duplicate emission prevention". Failed persistence is tested only on the async path, not sync or webhook. The duplicate test (:235) checks one poll only; no test shows that a later unchanged poll, or a reconciliation after a poll, emits nothing more.
• P3 "commanded … transitions". Only REMAIN_OPEN is tested (:150 and the JS engine test). REMAIN_CLOSED (3) is untested on both sides.
• P3 app/static/js/app.js:515 — "dashboard ingestion through the telemetry module". TelemetryEngine ingests transitions and they reach snapshot subscribers, but renderDoorsDashboard still renders only from data.overview, so the door grid waits for the next doors_update. Acceptable; say so explicitly in the PR.

(b) Scope creep

• P3 app/db/door_repository.py:266,661-689 — four INSERT copies become one _INSERT_TRANSITION_SQL, and executemany becomes per-row execute so ids come back. Needed for the transition id, so justified. Return type int → list[int] is fine; only the new helpers call it.
• docs/api/README.md gains a message-type table. Welcome, not creep.

(c) Implemented but questionable

• P3 app/db/door_repository.py:573,609 — single-row record_hardware_transition_sync / _async stay public with no callers. A future caller would persist without broadcasting, breaking "Cover all persistence paths". Remove or mark internal.
• P3 app/services/door_service.py:1404 — loop.create_task(...) keeps no reference and adds no done-callback, unlike _schedule_broadcast at :1450. (Same finding as Standards P2.)
• P3 app/services/door_service.py:1413 — exclusion is read from self.doors, so a door first seen in a poll always reports exclude_from_rankings=False, even when the DB row says it is excluded.

Open design points

Point	Status
Own message type vs riding in doors_update	Decided and sound: separate door_transitions message.
Where emission lives	Decided and sound: _record_transitions_async / _record_transitions_sync emit; repositories only persist.
"each newly persisted transition causes one emission attempt"	Decided and sound: one message per batch, each transition once. Loose reading of the wording, but the mapping holds.
Sync path with no running loop	Decided and sound: persisted, not broadcast; comment explains.
Replay after reconnect	Out of scope per triage. Initial snapshot on connect kept and tested (:258).

Test fidelity

Backend tests use real SQLite, check stored row id against broadcast id (:131), and assert the exact field set, proving no details_json or person data is sent (:135). They cover webhook, async and sync polling, reconciliation, offline/recovery and excluded doors. Engine tests cover ordering, late transitions, dedup, unknown doors, buffer cap and resync from the next overview — matching the PR's claims.

---

Summary: Standards 10 (1×P2, 9×P3) — worst: untracked create_task in _record_transitions_sync (webhook path). Spec 7 (all P3) — worst: orphaned public single-row transition writers bypass the broadcast.

🤖 Generated with Claude Code

# Code review — pass 1 (two-axis) Reviewed the PR's own diff (`git diff <base>...<head>`); Standards and Spec ran as independent passes and are reported separately, not reranked. Severity: **P1** must fix · **P2** fix before merge · **P3** minor. Per review policy, pass 1 findings are all to be fixed on the branch. ## Standards No P1 bugs. Layering holds: SQL stays in the repository (`_INSERT_TRANSITION_SQL` also consolidates four copies), the DTO lives in `app/schemas/`, services do the broadcasting, and each new path has a test. ### `app/services/door_service.py` - **P2, async hygiene (judgement call).** In `_record_transitions_sync` (~L1397-1405), `loop.create_task(self._broadcast_transitions(events))` discards the task handle. The loop holds only a weak reference (asyncio docs), so the task can be garbage-collected mid-run. The webhook path goes through here (called from the async `/webhook` route). Keep tasks in a set, or reuse the done-callback pattern. - **P3, possible Duplicated Code.** That block re-implements `_schedule_broadcast` (~L1450): `get_running_loop()` / `RuntimeError` / `create_task`, minus the done-callback. Extract one `_spawn(coro)` helper. The now-redundant local `import asyncio` in `_schedule_broadcast` (~L1453) can go, since this PR adds it at module top. - **P3, silent drop.** The `except RuntimeError` comment says "No event loop (CLI / sync callers): persisted, nothing to broadcast to". True only for CLI callers; a future caller on a worker thread (`to_thread`, threadpool `def` endpoint) would silently lose broadcasts while clients are connected. At least log at debug. - **P3, soft breach of `code-standards.md` §2.3** ("Avoid passing untyped, raw dictionaries between service layers when structured schemas are available"). The webhook hunk (~L858-880) builds a new 11-key `dict[str, Any]`, and `_transition_events` re-reads it key by key. Also possible **Data Clumps / Primitive Obsession**: the same transition fields travel as a dict through four paths. A typed input record (a `DoorStateTransition`-like model without `id`) would remove the `str()`/`int()` coercions. - **P3, pre-existing.** The async `/webhook` route still does blocking sync SQLite writes via `handle_webhook_event`, breaching §2.4 "Never call blocking synchronous functions within async routes". This PR adds one more sync write there. Worth an issue rather than a fix here. - **P3.** `door.get("exclusionSource") or door.get("exclusion_source")` (~L1425) is a dual-key fallback; pick one canonical key. ### `app/db/door_repository.py` - **P3.** `cursor.lastrowid` is `int | None`, but methods now declare `-> list[int]` (~L664-689). Add an assert or cast. ### `app/static/js/src/telemetry/telemetry_engine.js` - **P3, possible Repeated Switches.** `_ingestDoorTransitions` (~L641-667) adds a third copy of the door-state → open/closed/offline flag logic (others in `_ingestDoors` and the snapshot builder). - **P3.** Updates `stateKey` but leaves `stateLabel` stale (e.g. "🟢 Asegurada" on a now-open door) until the next overview. Nothing renders `stateLabel` today, so latent. ### PR description - **P3.** `docs/standards/git-and-workflow.md` §2.2 requires a **Checklist** section; none present. ## Spec **Verdict:** meets every acceptance criterion in the 2026-09-23 triage resolution. No P1 or P2 findings; the P3s are hardening and test gaps. ### (a) Missing or partial - **P3** `tests/test_door_transition_broadcast.py:222` — *"failed persistence, duplicate emission prevention"*. Failed persistence is tested only on the async path, not sync or webhook. The duplicate test (`:235`) checks one poll only; no test shows that a later unchanged poll, or a reconciliation after a poll, emits nothing more. - **P3** *"commanded … transitions"*. Only `REMAIN_OPEN` is tested (`:150` and the JS engine test). `REMAIN_CLOSED` (3) is untested on both sides. - **P3** `app/static/js/app.js:515` — *"dashboard ingestion through the telemetry module"*. `TelemetryEngine` ingests transitions and they reach snapshot subscribers, but `renderDoorsDashboard` still renders only from `data.overview`, so the door grid waits for the next `doors_update`. Acceptable; say so explicitly in the PR. ### (b) Scope creep - **P3** `app/db/door_repository.py:266,661-689` — four INSERT copies become one `_INSERT_TRANSITION_SQL`, and `executemany` becomes per-row `execute` so ids come back. Needed for the transition id, so justified. Return type `int` → `list[int]` is fine; only the new helpers call it. - `docs/api/README.md` gains a message-type table. Welcome, not creep. ### (c) Implemented but questionable - **P3** `app/db/door_repository.py:573,609` — single-row `record_hardware_transition_sync` / `_async` stay public with no callers. A future caller would persist without broadcasting, breaking *"Cover all persistence paths"*. Remove or mark internal. - **P3** `app/services/door_service.py:1404` — `loop.create_task(...)` keeps no reference and adds no done-callback, unlike `_schedule_broadcast` at `:1450`. (Same finding as Standards P2.) - **P3** `app/services/door_service.py:1413` — exclusion is read from `self.doors`, so a door first seen in a poll always reports `exclude_from_rankings=False`, even when the DB row says it is excluded. ### Open design points | Point | Status | | :--- | :--- | | Own message type vs riding in `doors_update` | Decided and sound: separate `door_transitions` message. | | Where emission lives | Decided and sound: `_record_transitions_async` / `_record_transitions_sync` emit; repositories only persist. | | *"each newly persisted transition causes one emission attempt"* | Decided and sound: one message per batch, each transition once. Loose reading of the wording, but the mapping holds. | | Sync path with no running loop | Decided and sound: persisted, not broadcast; comment explains. | | Replay after reconnect | Out of scope per triage. Initial snapshot on connect kept and tested (`:258`). | ### Test fidelity Backend tests use real SQLite, check stored row id against broadcast id (`:131`), and assert the exact field set, proving no `details_json` or person data is sent (`:135`). They cover webhook, async and sync polling, reconciliation, offline/recovery and excluded doors. Engine tests cover ordering, late transitions, dedup, unknown doors, buffer cap and resync from the next overview — matching the PR's claims. --- **Summary:** Standards 10 (1×P2, 9×P3) — worst: untracked `create_task` in `_record_transitions_sync` (webhook path). Spec 7 (all P3) — worst: orphaned public single-row transition writers bypass the broadcast. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added 1 commit 2026-09-25 09:22:22 +00:00

fix: retain transition broadcasts and close persistence bypasses 0beef06b42

gabogg added 1 commit 2026-09-25 09:25:31 +00:00

test: cover remain-closed transition ingestion ea7e01d5b7

gabogg added 1 commit 2026-09-25 09:49:05 +00:00

test: suppress broadcasts on sync persistence failure d8520973e5

gabogg referenced this pull request 2026-09-25 09:59:12 +00:00

fix(webhook): avoid synchronous SQLite writes in async event route #95

gabogg commented 2026-09-25 10:28:23 +00:00

Author

Owner

Copy link

Code review — pass 2 (two-axis)

Re-reviewed at head d852097. Each axis verified every pass-1 finding against the code (not the commit messages), then reviewed the fix commits for new problems. Verdicts: ✅ fixed · ◐ partial · ❌ not addressed · ↺ regressed · ⊘ declined with documented reason. Per review policy, from pass 2 on P1/P2 block merge; P3s go to one follow-up issue for this PR.

Standards

Verified at head (fix commits 0beef06, ea7e01d, d852097). In a throwaway worktree: ruff check and ruff format --check pass; pytest tests/test_door_transition_broadcast.py tests/test_analytics.py 18 passed; node --test tests/frontend/test_telemetry_engine.test.js 31 passed.

Pass-1 findings

Finding	Verdict	Evidence
P2: create_task handle discarded in _record_transitions_sync	✅ fixed	0beef06 door_service.py:1463-1470: task kept in self._broadcast_tasks (:94), done-callback discards it. Sound: no blocking, exceptions logged, cancellation handled.
P3: duplicated loop/create_task block; stray local import asyncio	✅ fixed	Both paths use _spawn_broadcast(make_coro) (:1403, :1461); only module-top import asyncio (:1) remains.
P3: broadcast silently dropped with no loop	✅ fixed	Logs at debug (:1477); see wording finding.
P3 §2.3: raw 11-key transition dicts across four paths	❌ not addressed	_transition_events (:1405-1440) still reads key by key with str()/int() coercions. No reason documented.
P3: pre-existing sync SQLite in async /webhook	⊘ declined, tracked	Description "Review follow-up": tracked by #95.
P3: dual-key exclusionSource / exclusion_source fallback	❌ not addressed	Still at :1435; the fix even builds a new dict with the camelCase key (:1416) to feed it.
P3: lastrowid is int | None	✅ fixed	assert cursor.lastrowid is not None at door_repository.py:601,617.
P3 possible Repeated Switches in engine state-flag logic	❌ not addressed	telemetry_engine.js:654-663 still maps state→flags inline, and now also state→label inline.
P3: stateLabel stale	◐ partial	ea7e01d telemetry_engine.js:663 sets it, but labels don't match the server's canonical DoorState.label (app/schemas/models.py:36-42).
P3: Checklist section missing	✅ fixed	## Checklist with three items.

New findings

• P3, app/services/door_service.py:1411-1422, possible Speculative Generality + sync I/O in an async path. The new fallback for doors missing from self.doors calls door_repo.get_by_code_sync(). _record_transitions_async goes through the same function, so this is a blocking SQLite read on the event loop — soft breach of §2.4 and AGENTS.md async hygiene; get_by_code_async exists. No current path appears to reach it (polling only records a transition when prev_door is in self.doors, :443-551; webhook and reconciliation also take the door from self.doors), and no test covers it. Delete it, or make it async and test it. (Spec axis reached the same conclusion independently.)
• P3, telemetry_engine.js:663: labels differ from the server's. Offline shows ⚫ Sin conexión (used nowhere else; server: ⚪ Fuera de Línea). REMAIN_OPEN/REMAIN_CLOSED collapse to 🟡 Abierta/🟢 Asegurada, while the server shows Permanecer Abierta/🔒 Permanecer Cerrada. A transition and the next overview give different labels for the same door.
• P3, door_service.py:1477: misleading log text. "Door broadcast deferred: no running event loop" implies a retry; nothing retries. Say skipped/dropped.
• P3, stale Verification section. Still says 9 backend + 9 engine tests, 291 passed; branch now has 11 backend, 10 engine, and a changed test_analytics.py. Refresh the counts (git-and-workflow.md asks for passing-test evidence).

Nothing new at P1/P2 in a re-scan of the full diff.

Merge readiness (this axis): Ready. P2 fixed; remaining P3s → follow-up issue.

Spec

Head d852097.

Pass-1 findings

Finding	Verdict	Evidence
P3: failed persistence tested only on async path	◐ partial (sufficient)	d852097 adds test_failed_sync_persistence_broadcasts_nothing (tests/test_door_transition_broadcast.py:245). It calls _record_transitions_sync directly with trigger_source="WEBHOOK" rather than through handle_webhook_event, but that is the webhook's only persistence seam (door_service.py:860), so this is enough.
P3: duplicate check covers one poll only	◐ partial	test_polling_broadcasts_remain_closed_once (:160) runs two polls and asserts one emission — "later unchanged poll" covered. Reconciliation after a poll still untested.
P3: REMAIN_CLOSED untested	✅ fixed	Backend :160 (1→3) plus engine test "remain-closed updates the state and label" (ea7e01d).
P3: dashboard grid renders only from overview; say so	✅ fixed	Stated in the description's "Review follow-up".
P3: repository refactor (scope)	✅ no action needed	Justified in pass 1.
P3: public single-row writers bypass broadcast	✅ fixed	0beef06 removes both from app/db/door_repository.py; tests/test_analytics.py moved to the batch API. Only INSERT left is _INSERT_TRANSITION_SQL (:267), called only by the two service helpers.
P3: untracked create_task	✅ fixed	_spawn_broadcast (door_service.py:1463-1479) keeps tasks in _broadcast_tasks, discards on completion; _record_transitions_sync (:1399-1403) uses it.
P3: first-seen door reports exclude_from_rankings=False	✅ fixed, but on a false premise (see N1)	door_service.py:1411-1422 falls back to door_repo.get_by_code_sync.

New findings

• P3, N1 app/services/door_service.py:1411-1422: the new DB fallback in _transition_events never runs. Every path creates a transition only for a door already in self.doors: async poll (:443-444 needs prev_door, :551 needs prev_state is not None), sync poll (:632-633), webhook (if door:, :856), reconciliation (loops over known doors). So pass-1's first-seen case can't happen, and the fix adds an untested branch. If it ever ran on the async poll or reconciliation path it would be a blocking sync SQLite call inside an async function (code-standards §2.4); spec: "Services own emission after successful persistence". Remove the branch, or make it async and test it. (The pass-1 finding rested on a false premise.)
• P3, N2: the description's Verification section is stale: says 9 backend + 9 engine tests; there are now 11 backend (:123-294) and 10 engine transition tests.
• P3, N3: spec "duplicate emission prevention" — no test shows a reconciliation right after a poll that already recorded the change emits nothing more.

No P1/P2. Fix commits cause no spec drift: emission only after persistence; one message per batch, no details_json or person data; excluded doors still included; reconnect snapshot kept and tested (:294).

Tests run in a throwaway worktree: tests/test_door_transition_broadcast.py + tests/test_analytics.py 18 passed; tests/frontend/test_telemetry_engine.test.js 31 passed.

Merge readiness (this axis): Ready. Remaining P3s (N1–N3) → follow-up issue.

---

Summary: Ready on both axes (no P1/P2). Both axes independently flag the new get_by_code_sync fallback in _transition_events as unreachable and, if reached, blocking I/O on the loop (P3). Worst per axis: Standards → that fallback; Spec → same, plus stale test counts in the description.

🤖 Generated with Claude Code

# Code review — pass 2 (two-axis) Re-reviewed at head `d852097`. Each axis verified every pass-1 finding against the code (not the commit messages), then reviewed the fix commits for new problems. Verdicts: ✅ fixed · ◐ partial · ❌ not addressed · ↺ regressed · ⊘ declined with documented reason. Per review policy, from pass 2 on **P1/P2 block merge; P3s go to one follow-up issue** for this PR. ## Standards Verified at head (fix commits 0beef06, ea7e01d, d852097). In a throwaway worktree: `ruff check` and `ruff format --check` pass; `pytest tests/test_door_transition_broadcast.py tests/test_analytics.py` 18 passed; `node --test tests/frontend/test_telemetry_engine.test.js` 31 passed. ### Pass-1 findings | Finding | Verdict | Evidence | | :--- | :--- | :--- | | P2: `create_task` handle discarded in `_record_transitions_sync` | ✅ fixed | 0beef06 `door_service.py:1463-1470`: task kept in `self._broadcast_tasks` (`:94`), done-callback discards it. Sound: no blocking, exceptions logged, cancellation handled. | | P3: duplicated loop/`create_task` block; stray local `import asyncio` | ✅ fixed | Both paths use `_spawn_broadcast(make_coro)` (`:1403`, `:1461`); only module-top `import asyncio` (`:1`) remains. | | P3: broadcast silently dropped with no loop | ✅ fixed | Logs at debug (`:1477`); see wording finding. | | P3 §2.3: raw 11-key transition dicts across four paths | ❌ not addressed | `_transition_events` (`:1405-1440`) still reads key by key with `str()`/`int()` coercions. No reason documented. | | P3: pre-existing sync SQLite in async `/webhook` | ⊘ declined, tracked | Description "Review follow-up": tracked by #95. | | P3: dual-key `exclusionSource` / `exclusion_source` fallback | ❌ not addressed | Still at `:1435`; the fix even builds a new dict with the camelCase key (`:1416`) to feed it. | | P3: `lastrowid` is `int \| None` | ✅ fixed | `assert cursor.lastrowid is not None` at `door_repository.py:601,617`. | | P3 possible Repeated Switches in engine state-flag logic | ❌ not addressed | `telemetry_engine.js:654-663` still maps state→flags inline, and now also state→label inline. | | P3: `stateLabel` stale | ◐ partial | ea7e01d `telemetry_engine.js:663` sets it, but labels don't match the server's canonical `DoorState.label` (`app/schemas/models.py:36-42`). | | P3: Checklist section missing | ✅ fixed | `## Checklist` with three items. | ### New findings - **P3, `app/services/door_service.py:1411-1422`, possible Speculative Generality + sync I/O in an async path.** The new fallback for doors missing from `self.doors` calls `door_repo.get_by_code_sync()`. `_record_transitions_async` goes through the same function, so this is a blocking SQLite read on the event loop — soft breach of §2.4 and AGENTS.md async hygiene; `get_by_code_async` exists. No current path appears to reach it (polling only records a transition when `prev_door` is in `self.doors`, `:443-551`; webhook and reconciliation also take the door from `self.doors`), and no test covers it. Delete it, or make it async and test it. (Spec axis reached the same conclusion independently.) - **P3, `telemetry_engine.js:663`: labels differ from the server's.** Offline shows `⚫ Sin conexión` (used nowhere else; server: `⚪ Fuera de Línea`). `REMAIN_OPEN`/`REMAIN_CLOSED` collapse to `🟡 Abierta`/`🟢 Asegurada`, while the server shows `Permanecer Abierta`/`🔒 Permanecer Cerrada`. A transition and the next overview give different labels for the same door. - **P3, `door_service.py:1477`: misleading log text.** "Door broadcast deferred: no running event loop" implies a retry; nothing retries. Say skipped/dropped. - **P3, stale Verification section.** Still says 9 backend + 9 engine tests, 291 passed; branch now has 11 backend, 10 engine, and a changed `test_analytics.py`. Refresh the counts (git-and-workflow.md asks for passing-test evidence). Nothing new at P1/P2 in a re-scan of the full diff. **Merge readiness (this axis):** Ready. P2 fixed; remaining P3s → follow-up issue. ## Spec Head d852097. ### Pass-1 findings | Finding | Verdict | Evidence | | :--- | :--- | :--- | | P3: failed persistence tested only on async path | ◐ partial (sufficient) | d852097 adds `test_failed_sync_persistence_broadcasts_nothing` (`tests/test_door_transition_broadcast.py:245`). It calls `_record_transitions_sync` directly with `trigger_source="WEBHOOK"` rather than through `handle_webhook_event`, but that is the webhook's only persistence seam (`door_service.py:860`), so this is enough. | | P3: duplicate check covers one poll only | ◐ partial | `test_polling_broadcasts_remain_closed_once` (`:160`) runs two polls and asserts one emission — "later unchanged poll" covered. Reconciliation after a poll still untested. | | P3: `REMAIN_CLOSED` untested | ✅ fixed | Backend `:160` (1→3) plus engine test "remain-closed updates the state and label" (ea7e01d). | | P3: dashboard grid renders only from overview; say so | ✅ fixed | Stated in the description's "Review follow-up". | | P3: repository refactor (scope) | ✅ no action needed | Justified in pass 1. | | P3: public single-row writers bypass broadcast | ✅ fixed | 0beef06 removes both from `app/db/door_repository.py`; `tests/test_analytics.py` moved to the batch API. Only INSERT left is `_INSERT_TRANSITION_SQL` (`:267`), called only by the two service helpers. | | P3: untracked `create_task` | ✅ fixed | `_spawn_broadcast` (`door_service.py:1463-1479`) keeps tasks in `_broadcast_tasks`, discards on completion; `_record_transitions_sync` (`:1399-1403`) uses it. | | P3: first-seen door reports `exclude_from_rankings=False` | ✅ fixed, but on a false premise (see N1) | `door_service.py:1411-1422` falls back to `door_repo.get_by_code_sync`. | ### New findings - **P3, N1** `app/services/door_service.py:1411-1422`: the new DB fallback in `_transition_events` never runs. Every path creates a transition only for a door already in `self.doors`: async poll (`:443-444` needs `prev_door`, `:551` needs `prev_state is not None`), sync poll (`:632-633`), webhook (`if door:`, `:856`), reconciliation (loops over known doors). So pass-1's first-seen case can't happen, and the fix adds an untested branch. If it ever ran on the async poll or reconciliation path it would be a blocking sync SQLite call inside an async function (code-standards §2.4); spec: *"Services own emission after successful persistence"*. Remove the branch, or make it async and test it. (The pass-1 finding rested on a false premise.) - **P3, N2**: the description's Verification section is stale: says 9 backend + 9 engine tests; there are now 11 backend (`:123-294`) and 10 engine transition tests. - **P3, N3**: spec *"duplicate emission prevention"* — no test shows a reconciliation right after a poll that already recorded the change emits nothing more. No P1/P2. Fix commits cause no spec drift: emission only after persistence; one message per batch, no `details_json` or person data; excluded doors still included; reconnect snapshot kept and tested (`:294`). Tests run in a throwaway worktree: `tests/test_door_transition_broadcast.py` + `tests/test_analytics.py` 18 passed; `tests/frontend/test_telemetry_engine.test.js` 31 passed. **Merge readiness (this axis):** Ready. Remaining P3s (N1–N3) → follow-up issue. --- **Summary:** **Ready on both axes** (no P1/P2). Both axes independently flag the new `get_by_code_sync` fallback in `_transition_events` as unreachable and, if reached, blocking I/O on the loop (P3). Worst per axis: Standards → that fallback; Spec → same, plus stale test counts in the description. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg referenced this pull request 2026-09-25 12:14:48 +00:00

follow-up(telemetry): P3 cleanups from #94 review (door transition broadcast) #102

gabogg added 1 commit 2026-09-25 12:19:38 +00:00

Merge remote-tracking branch 'origin/master' into feat/telemetry-door-transition-broadcast ... 20e03afd4f

# Conflicts:
#	app/services/door_service.py

gabogg merged commit 634db84cfe into master 2026-09-25 12:19:43 +00:00

gabogg deleted branch feat/telemetry-door-transition-broadcast 2026-09-25 12:19:43 +00:00

gabogg referenced this pull request from a commit 2026-09-25 12:19:45 +00:00

feat(telemetry): broadcast door state transitions as their own message (#55) (#94)

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

Subscribe

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!94

No description provided.