gabogg/hikcentral

Watch

Fork 0

fix(telemetry): show the cardholder name for card-opened doors (#46) #93

Merged

gabogg merged 4 commits from fix/telemetry-open-longest-cardholder into master

2026-09-25 12:17:46 +00:00

gabogg commented

2026-09-25 01:19:45 +00:00

Owner

Closes #46

Problem

In Doors Opened the Longest, a card-opened door showed TARJETA: <id> with no name, while Live View showed the cardholder's name.

Root cause, confirmed on production (read-only, 2026-09-24):

Door webhooks (OnEventNotify, event_acs) carry data.cardNo, data.personId and data.personCode, but no personName. This is from the production gateway log, for example {'cardNo': '4267114340', 'personId': '65', 'personCode': '25108016', …}.
The webhook path therefore creates the active-open cycle with a card number and an empty name. door_access_cycles in production holds about 4,900 card-without-name cycle-* rows, 5 of them open at the time of the check.
The named rows (event-hc-*, 8,112 of them) come from the door/events poll, which does return personName. Live View shows those rows, so it has the name.

The design comment on #46 placed personId on the door/events poll. In fact it is the webhook that carries the id without the name, so this PR captures it there (and from the poll too, where present).

Approach

Capture the person id. door_access_cycles gets a new person_id column. It is created in the table definition and added in place on existing databases, together with an index. The webhook handler and both door/events polls read personId and pass it through record_access_session into the aggregator.
Keep the credential on an open cycle. The aggregator now copies cardNo and personId onto an already-open cycle that lacks them. Before, it did so only when the event also had a name. This covers the common order: the state poll opens an anonymous cycle, then the card webhook arrives.
Resolve the name once, in the background. New app/services/person_directory.py (PersonDirectory):
- cached_name(personId) returns the name, '' when HikCentral has none, or None if it hasn't been asked yet.
- request(personId, on_resolved) starts at most one background lookup per person through POST /artemis/api/resource/v1/person/personId/personInfo with {"personId": …}. I checked the response shape live: data.data.personName.
- After a failed lookup, that person isn't retried for 5 minutes, so an outage never becomes one request per poll.
- get_door_overview() is synchronous and runs on every poll, so it never waits on HikCentral. On a cache miss it shows the card only for that poll and schedules the lookup.
Write the name back. cycle_repo.fill_person_name_async sets the name on every nameless cycle of that person, so the ranking and the live activity stream agree. The door service then broadcasts a fresh overview.

The frontend is unchanged: it already renders PERSONA: whenever personName is set, through i18n in both languages.

Verification

New tests/test_cardholder_name_resolution.py (7 tests), with HikCentral stubbed and a real SQLite database:

a card webhook shaped like production's stores personId and cardNo on the open cycle;
the first overview doesn't wait; after the lookup, open-longest shows the name with trigger CREDENTIAL; 6 overviews make one HikCentral request;
the written-back name reaches the active cycle and the recent-activity list;
a failed lookup isn't repeated on every poll and is retried after the pause;
a button-opened door makes no lookup and keeps trigger BUTTON;
the "poll first, card webhook second" order keeps the identity;
an old database gains the person_id column when init_db runs.

Full suite: 289 passed, 1 skipped. ruff check and ruff format are clean, and pre-commit passed.

Notes for review

Where the lookup runs: PersonDirectory calls HikCentral itself, rather than going through a repository. It sits in services/ like the other callers of artemis.
Person id in exports: person_id is HikCentral's internal id. The sanitizer masks card_no in exported copies but not person_id. Masking it too would be a one-line change if you want it.
Summary labels: they are not rewritten; the Spanish strings are left to the i18n sweep in #73.
Deploy: the migration adds one column and an index. Cycles recorded before the deploy have no person_id, so the fix applies to openings after the deploy.

🤖 Generated with Claude Code

Checklist

Person lookup tasks stay referenced until complete.
Cardholder names update active cycles after lookup.
Sanitized database copies clear person ids.
Ruff and full pytest pass.

Review follow-up

Background name lookup tasks are retained until complete, and exported database copies clear person_id. The person-info response used here provides a name; a card-only lookup does not invent a role. The remaining localized summaryLabel behavior is recorded on #73 for the language-agnostic payload work.

Closes #46 ## Problem In **Doors Opened the Longest**, a card-opened door showed `TARJETA: <id>` with no name, while Live View showed the cardholder's name. **Root cause, confirmed on production (read-only, 2026-09-24):** - Door webhooks (`OnEventNotify`, `event_acs`) carry `data.cardNo`, `data.personId` and `data.personCode`, but **no `personName`**. This is from the production gateway log, for example `{'cardNo': '4267114340', 'personId': '65', 'personCode': '25108016', …}`. - The webhook path therefore creates the active-open cycle with a card number and an empty name. `door_access_cycles` in production holds about 4,900 card-without-name `cycle-*` rows, 5 of them open at the time of the check. - The named rows (`event-hc-*`, 8,112 of them) come from the `door/events` poll, which does return `personName`. Live View shows those rows, so it has the name. The design comment on #46 placed `personId` on the `door/events` poll. In fact it is the **webhook** that carries the id without the name, so this PR captures it there (and from the poll too, where present). ## Approach 1. **Capture the person id.** `door_access_cycles` gets a new `person_id` column. It is created in the table definition and added in place on existing databases, together with an index. The webhook handler and both `door/events` polls read `personId` and pass it through `record_access_session` into the aggregator. 2. **Keep the credential on an open cycle.** The aggregator now copies `cardNo` and `personId` onto an already-open cycle that lacks them. Before, it did so only when the event also had a name. This covers the common order: the state poll opens an anonymous cycle, then the card webhook arrives. 3. **Resolve the name once, in the background.** New `app/services/person_directory.py` (`PersonDirectory`): - `cached_name(personId)` returns the name, `''` when HikCentral has none, or `None` if it hasn't been asked yet. - `request(personId, on_resolved)` starts at most one background lookup per person through `POST /artemis/api/resource/v1/person/personId/personInfo` with `{"personId": …}`. I checked the response shape live: `data.data.personName`. - After a failed lookup, that person isn't retried for 5 minutes, so an outage never becomes one request per poll. - `get_door_overview()` is synchronous and runs on every poll, so it never waits on HikCentral. On a cache miss it shows the card only for that poll and schedules the lookup. 4. **Write the name back.** `cycle_repo.fill_person_name_async` sets the name on every nameless cycle of that person, so the ranking and the live activity stream agree. The door service then broadcasts a fresh overview. The frontend is unchanged: it already renders `PERSONA:` whenever `personName` is set, through i18n in both languages. ## Verification New `tests/test_cardholder_name_resolution.py` (7 tests), with HikCentral stubbed and a real SQLite database: - a card webhook shaped like production's stores `personId` and `cardNo` on the open cycle; - the first overview doesn't wait; after the lookup, open-longest shows the name with trigger `CREDENTIAL`; 6 overviews make **one** HikCentral request; - the written-back name reaches the active cycle and the recent-activity list; - a failed lookup isn't repeated on every poll and is retried after the pause; - a button-opened door makes no lookup and keeps trigger `BUTTON`; - the "poll first, card webhook second" order keeps the identity; - an old database gains the `person_id` column when `init_db` runs. Full suite: **289 passed, 1 skipped**. `ruff check` and `ruff format` are clean, and pre-commit passed. ## Notes for review - **Where the lookup runs:** `PersonDirectory` calls HikCentral itself, rather than going through a repository. It sits in `services/` like the other callers of `artemis`. - **Person id in exports:** `person_id` is HikCentral's internal id. The sanitizer masks `card_no` in exported copies but not `person_id`. Masking it too would be a one-line change if you want it. - **Summary labels:** they are not rewritten; the Spanish strings are left to the i18n sweep in #73. - **Deploy:** the migration adds one column and an index. Cycles recorded before the deploy have no `person_id`, so the fix applies to openings after the deploy. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Checklist - [x] Person lookup tasks stay referenced until complete. - [x] Cardholder names update active cycles after lookup. - [x] Sanitized database copies clear person ids. - [x] Ruff and full pytest pass. ## Review follow-up Background name lookup tasks are retained until complete, and exported database copies clear `person_id`. The person-info response used here provides a name; a card-only lookup does not invent a role. The remaining localized `summaryLabel` behavior is recorded on #73 for the language-agnostic payload work.

gabogg added 1 commit

2026-09-25 01:19:45 +00:00

fix(telemetry): show the cardholder name for card-opened doors (#46 )

CI / lint-and-test (pull_request) Successful in 1m27s

Details

18d747dad9

Door webhooks carry the card number and HikCentral person id but no name,
so a card-opened door's active cycle had a card and no person, and Doors
Opened the Longest showed only the card ID.

- Capture personId from door webhooks and the door/events poll and store
  it on the cycle (new door_access_cycles.person_id column, migrated in
  place). The aggregator now keeps card number and person id on an open
  cycle even when the event carries no name.
- New PersonDirectory resolves a person id through HikCentral's
  person/personId/personInfo once per person, in the background, with a
  5-minute pause after a failed lookup. The overview never waits on it.
- A resolved name is written back onto every nameless cycle of that
  person, so the open-longest ranking and the live activity stream agree,
  and a fresh overview is broadcast.

Closes #46

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

gabogg referenced this pull request

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

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

gabogg commented

2026-09-25 08:39:19 +00:00

Author

Owner

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

Layering and migration are fine. All SQL stays in app/db/: fill_person_name_async is a repository method, and the migration only reads columns, calls ALTER and creates the index in init_schema, matching how the file already handles door_records and occupancy_config. ADD COLUMN ... TEXT DEFAULT '' plus CREATE INDEX IF NOT EXISTS is re-runnable and non-blocking in SQLite. The Artemis call is mocked, and the migration test runs against a real on-disk database. No hard breaches of the documented standards.

Bugs

P2 app/services/person_directory.py:57: loop.create_task(self._resolve_and_report(...)) keeps no reference to the task. The event loop holds only weak references to tasks (asyncio docs), so an untracked task can be garbage-collected before it finishes. Then the finally never runs and the person id stays in _in_flight for good — that person is never looked up again. Keep tasks in a set and remove with add_done_callback(set.discard).
P3 person_directory.py:81: res.get("success") runs before the isinstance(res, dict) guard. Non-dict res would raise AttributeError instead of PersonLookupError. artemis.request_async always returns a dict, so it's guard order, not a live bug. Check the type first or drop the guards.

Judgement calls (baseline smells)

P3, possible Duplicated Code: person_id = str(cycle.get("personId") or cycle.get("person_id") or "") in both insert paths (cycle_repository.py:101, :160), and person_id = str(ev.get("personId") or "") in both door/events polls (door_service.py:292, :372). Extends pre-existing duplication.
P3, possible Data Clumps / Shotgun Surgery: one credential field touched six files, because cardNo, personId, personName, personRole and picUrl travel as loose params/dict keys. A small Credential value would absorb the next field. Related to code-standards §2.3 ("avoid passing untyped, raw dictionaries…"), but the cycle dicts pre-exist, so not a new breach.
P3, possible Duplicated Code: access_cycle_aggregator.py:87 now copies cardNo whenever missing, making the older updated["cardNo"] = ... at :93 in the has_person branch mostly redundant.
P3: PersonDirectory._names (:33) caches every name forever; a renamed person keeps the old name until restart. Acceptable at mall scale, but say so in the docstring.

Other notes

P3, privacy: sanitizer_repository.py:45 and :97 mask card_no in exported copies but not the new person_id. The PR raises this already; it's a one-line change and keeps credentials treated consistently.
P3: fill_person_name_async fills person_name but not person_role. Poll-sourced Live View rows carry a role, so card-resolved rows will show a name without a role.
P3, PR description: git-and-workflow.md §2.2 asks for a Checklist section; none present.

Spec

The PR does what the settled design comment on #46 asked for, with a small, justified deviation. No P1 or P2 issues.

Deviation from the design comment: decided and sound

"Capture personId … from acs/v1/door/events during ingestion"

The PR captures personId from the webhook (app/services/door_service.py:822) and from both door/events polls (:292, :372). Production evidence: the webhook carries personId/personCode with no personName, and poll rows are already named. So the nameless row really comes from the webhook, and the change follows the spec's intent ("capture the key") where the key is actually missing. "Validate the exact field name" is done: personId, response shape checked live (data.data.personName).

Settled points

Spec line	Verdict
"carried onto cycle rows alongside `cardNo`"	Done: new column + migration (`app/db/database.py:160`), aggregator merge (`app/services/access_cycle_aggregator.py:84-88`)
"resolves a blank name via `person/personId/personInfo`, cached onto the cycle"	Done: `app/services/person_directory.py`, write-back in `app/db/cycle_repository.py:203`
"No per-poll HikCentral request"	Done: in-memory cache + in-flight guard. Tested (`tests/test_cardholder_name_resolution.py:161-164`)
"Drop the local-list match, 300 s window, re-query fallback"	None added
"Frontend unchanged"	Confirmed, no JS in the diff
"Button / manual / forced / unknown triggers unaffected"	Partial. Only BUTTON is tested (`:202`)

(a) Missing or partial

P3, "Verified across Spanish and English localizations": no evidence in either language. Frontend unchanged, so low risk, but the checkbox isn't backed.
P3, "Button / manual / forced / unknown triggers unaffected": FORCED is exposed — an alarm event carrying a card now copies cardNo/personId onto the cycle (access_cycle_aggregator.py:84-87). No forced-open test; add one asserting FORCED still wins.

(b) Scope creep

P3, app/services/person_directory.py:14: the 5-minute retry after a failed lookup wasn't asked for. Reasonable way to meet "no per-poll request" during an outage — keep it. _names has no size limit (grows with distinct cardholders).
The broadcast after write-back (app/services/door_service.py:1404-1408) is needed so the panel updates without waiting for the next state change. Fine.

(c) Implemented, but looks wrong or untested

P3, tests/test_cardholder_name_resolution.py:220-236: the "poll first, card webhook second" test checks cardNo/personId but not that open_trigger moves BUTTON → CREDENTIAL, nor that the name resolves in that order. Code handles it via the elif open_trigger branch; assert it.
P3, app/services/access_cycle_aggregator.py:39-41: a card-only cycle keeps summaryLabel "Salida: Botón de Apertura / Sensor" after its name resolves, and the panel reads summaryLabel (command_deck_adapter.js:838). PR defers to #73 — record that in #73 so it isn't lost.
Rows recorded by door/events with a name but no personId are unaffected (correct). Pre-deploy cycles stay nameless, as the PR notes.

Tests cover the claims: capture, no waiting, one request per person, write-back reaching recent activity, retry after failure. (Suite not run; review was read-only.)

Summary: Standards 9 (1×P2, 8×P3) — worst: untracked create_task in PersonDirectory can be GC'd and permanently wedge _in_flight. Spec 4 (all P3) — worst: no FORCED-trigger test now that alarm events copy card/person ids.

🤖 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 **Layering and migration are fine.** All SQL stays in `app/db/`: `fill_person_name_async` is a repository method, and the migration only reads columns, calls `ALTER` and creates the index in `init_schema`, matching how the file already handles `door_records` and `occupancy_config`. `ADD COLUMN ... TEXT DEFAULT ''` plus `CREATE INDEX IF NOT EXISTS` is re-runnable and non-blocking in SQLite. The Artemis call is mocked, and the migration test runs against a real on-disk database. No hard breaches of the documented standards. ### Bugs - **P2** `app/services/person_directory.py:57`: `loop.create_task(self._resolve_and_report(...))` keeps no reference to the task. The event loop holds only weak references to tasks (asyncio docs), so an untracked task can be garbage-collected before it finishes. Then the `finally` never runs and the person id stays in `_in_flight` for good — that person is never looked up again. Keep tasks in a set and remove with `add_done_callback(set.discard)`. - **P3** `person_directory.py:81`: `res.get("success")` runs before the `isinstance(res, dict)` guard. Non-dict `res` would raise `AttributeError` instead of `PersonLookupError`. `artemis.request_async` always returns a dict, so it's guard order, not a live bug. Check the type first or drop the guards. ### Judgement calls (baseline smells) - **P3, possible Duplicated Code**: `person_id = str(cycle.get("personId") or cycle.get("person_id") or "")` in both insert paths (`cycle_repository.py:101`, `:160`), and `person_id = str(ev.get("personId") or "")` in both `door/events` polls (`door_service.py:292`, `:372`). Extends pre-existing duplication. - **P3, possible Data Clumps / Shotgun Surgery**: one credential field touched six files, because `cardNo`, `personId`, `personName`, `personRole` and `picUrl` travel as loose params/dict keys. A small `Credential` value would absorb the next field. Related to code-standards §2.3 ("avoid passing untyped, raw dictionaries…"), but the cycle dicts pre-exist, so not a new breach. - **P3, possible Duplicated Code**: `access_cycle_aggregator.py:87` now copies `cardNo` whenever missing, making the older `updated["cardNo"] = ...` at `:93` in the `has_person` branch mostly redundant. - **P3**: `PersonDirectory._names` (`:33`) caches every name forever; a renamed person keeps the old name until restart. Acceptable at mall scale, but say so in the docstring. ### Other notes - **P3, privacy**: `sanitizer_repository.py:45` and `:97` mask `card_no` in exported copies but not the new `person_id`. The PR raises this already; it's a one-line change and keeps credentials treated consistently. - **P3**: `fill_person_name_async` fills `person_name` but not `person_role`. Poll-sourced Live View rows carry a role, so card-resolved rows will show a name without a role. - **P3, PR description**: git-and-workflow.md §2.2 asks for a **Checklist** section; none present. ## Spec The PR does what the settled design comment on #46 asked for, with a small, justified deviation. No P1 or P2 issues. ### Deviation from the design comment: decided and sound > "Capture `personId` … from `acs/v1/door/events` during ingestion" The PR captures `personId` from the **webhook** (`app/services/door_service.py:822`) and from both `door/events` polls (`:292`, `:372`). Production evidence: the webhook carries `personId`/`personCode` with no `personName`, and poll rows are already named. So the nameless row really comes from the webhook, and the change follows the spec's intent ("capture the key") where the key is actually missing. "Validate the exact field name" is done: `personId`, response shape checked live (`data.data.personName`). ### Settled points | Spec line | Verdict | |---|---| | "carried onto cycle rows alongside `cardNo`" | Done: new column + migration (`app/db/database.py:160`), aggregator merge (`app/services/access_cycle_aggregator.py:84-88`) | | "resolves a blank name via `person/personId/personInfo`, cached onto the cycle" | Done: `app/services/person_directory.py`, write-back in `app/db/cycle_repository.py:203` | | "No per-poll HikCentral request" | Done: in-memory cache + in-flight guard. Tested (`tests/test_cardholder_name_resolution.py:161-164`) | | "Drop the local-list match, 300 s window, re-query fallback" | None added | | "Frontend unchanged" | Confirmed, no JS in the diff | | "Button / manual / forced / unknown triggers unaffected" | Partial. Only BUTTON is tested (`:202`) | ### (a) Missing or partial - **P3**, "Verified across Spanish and English localizations": no evidence in either language. Frontend unchanged, so low risk, but the checkbox isn't backed. - **P3**, "Button / manual / forced / unknown triggers unaffected": FORCED is exposed — an alarm event carrying a card now copies `cardNo`/`personId` onto the cycle (`access_cycle_aggregator.py:84-87`). No forced-open test; add one asserting FORCED still wins. ### (b) Scope creep - **P3**, `app/services/person_directory.py:14`: the 5-minute retry after a failed lookup wasn't asked for. Reasonable way to meet "no per-poll request" during an outage — keep it. `_names` has no size limit (grows with distinct cardholders). - The broadcast after write-back (`app/services/door_service.py:1404-1408`) is needed so the panel updates without waiting for the next state change. Fine. ### (c) Implemented, but looks wrong or untested - **P3**, `tests/test_cardholder_name_resolution.py:220-236`: the "poll first, card webhook second" test checks `cardNo`/`personId` but not that `open_trigger` moves BUTTON → CREDENTIAL, nor that the name resolves in that order. Code handles it via the `elif open_trigger` branch; assert it. - **P3**, `app/services/access_cycle_aggregator.py:39-41`: a card-only cycle keeps `summaryLabel` "Salida: Botón de Apertura / Sensor" after its name resolves, and the panel reads `summaryLabel` (`command_deck_adapter.js:838`). PR defers to #73 — record that in #73 so it isn't lost. - Rows recorded by `door/events` with a name but no `personId` are unaffected (correct). Pre-deploy cycles stay nameless, as the PR notes. Tests cover the claims: capture, no waiting, one request per person, write-back reaching recent activity, retry after failure. (Suite not run; review was read-only.) --- **Summary:** Standards 9 (1×P2, 8×P3) — worst: untracked `create_task` in `PersonDirectory` can be GC'd and permanently wedge `_in_flight`. Spec 4 (all P3) — worst: no FORCED-trigger test now that alarm events copy card/person ids. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added 1 commit

2026-09-25 09:14:21 +00:00

fix: retain person lookups and sanitize person ids

CI / lint-and-test (pull_request) Successful in 1m21s

Details

90d3841566

gabogg referenced this pull request

2026-09-25 09:43:15 +00:00

refactor(i18n): language-agnostic identifiers and payloads — i18n with English fallback, not a Spanish default #73

gabogg added 1 commit

2026-09-25 09:45:06 +00:00

test: keep forced-open trigger when a card is present

CI / lint-and-test (pull_request) Successful in 1m26s

Details

5ac58b0b0b

gabogg commented

2026-09-25 10:28:22 +00:00

Author

Owner

Code review — pass 2 (two-axis)

Re-reviewed at head 5ac58b0. 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

Checked 90d3841 and 5ac58b0 at head 5ac58b0. In a throwaway worktree, tests/test_cardholder_name_resolution.py + tests/test_database_sync.py pass (24), ruff check clean. Migration unchanged since pass 1 and still safe: checks columns before ALTER ... ADD COLUMN person_id TEXT DEFAULT '', uses CREATE INDEX IF NOT EXISTS (app/db/database.py:160-166).

Pass-1 findings

Finding	Verdict	Evidence
P2 untracked `create_task`	✅ fixed	`90d3841`, `person_directory.py:37,59-61`: tasks held in `self._tasks`, removed via `add_done_callback(self._tasks.discard)`; `_in_flight` still cleared in `finally` (`:71-72`).
P3 guard order in `fetch_name_async`	✅ fixed	`90d3841`, `person_directory.py:85-92`: `isinstance(res, dict)` first.
P3 duplicated `person_id` extraction (`cycle_repository.py:101,160`; `door_service.py:292,372`)	❌ not addressed	Unchanged. Judgement call → follow-up issue.
P3 Data Clumps / Shotgun Surgery (loose credential fields)	❌ not addressed	Unchanged. Judgement call → follow-up issue.
P3 redundant `cardNo` assignment in `has_person` branch	✅ fixed	`90d3841` removes it (`access_cycle_aggregator.py:86-95`). Side benefit: a named event with no card no longer wipes an existing `cardNo`.
P3 unbounded name cache undocumented	✅ fixed	`person_directory.py:29` docstring: cached until restart.
P3 `person_id` not masked in exports	✅ fixed in code, untested (see N1)	`sanitizer_repository.py:52-57,111-116` sets `person_id = ''` in sync and async paths.
P3 `fill_person_name_async` doesn't fill `person_role`	⊘ declined with reason	"Review follow-up": person-info lookup yields a name; card-only lookup won't invent a role.
P3 no Checklist section	✅ fixed	`## Checklist` with four ticked items.

New findings

P2 app/db/sanitizer_repository.py:52-57,111-116: person_id masking has no test. code-standards §4.3: "Every new feature, endpoint, or service fix must have accompanying tests." The checklist ticks "Sanitized database copies clear person ids", but nothing checks it. The existing tests/test_database_sync.py:95 seeds door_access_cycles without a person_id column, so the new UPDATE silently hits OperationalError (logged at debug) and the test still passes. A silent regression would leak ids into exported copies. Add person_id to the seeded table and assert '' after sanitizing.
P3 app/db/cycle_repository.py:203-214: fill_person_name_async has no guard for empty person_id. Today the only caller filters '' (person_directory.py:49, door_service.py:1396). A future call with '' would write one name onto every nameless cycle with no person id (~4,900 prod rows). Return 0 when empty.
P3, possible Duplicated Code: sanitizer_repository.py repeats the try/UPDATE/debug block in sync and async paths. Existing file pattern; note only.

Nothing else at P1/P2.

Merge readiness (this axis): One P2 left — the missing sanitizer test for person_id (a few lines). P3s → follow-up issue.

Spec

Reviewed origin/master...origin/fix/telemetry-open-longest-cardholder at head 5ac58b0, mainly fix commits 90d3841 and 5ac58b0 against #46's settled design comment. Throwaway worktree: tests/test_cardholder_name_resolution.py 8 passed; full suite 290 passed, 1 skipped.

Pass-1 findings

Finding	Verdict	Evidence
P3 "Verified across Spanish and English localizations" not evidenced	⊘ declined-with-reason	Description "Review follow-up" and #73 comment 2048 ("This also covers English and Spanish verification for the cardholder-name flow") hand this to #73. Frontend unchanged; low risk.
P3 FORCED exposed: alarm event with a card copies `cardNo`/`personId`; test FORCED wins	◐ partial	`5ac58b0` adds `test_forced_open_with_card_keeps_forced_trigger` (`tests/test_cardholder_name_resolution.py:243-252`), asserting `cardNo`, `personId`, `open_trigger == "FORCED"`. But the door starts closed, so the event takes the create path (`access_cycle_aggregator.py:51`). The copy lines the finding pointed at (`:84-87`, `else` branch, existing cycle) remain unexercised. Reading the code, FORCED still wins there (`:101-104` sets FORCED when `personName` is blank), but no test pins it.
P3 retry pause and unbounded `_names` cache	✅	`90d3841` docstring: "Resolved names remain cached until process restart" (`person_directory.py:29`). No size limit, accepted in pass 1.
P3 "Poll first, card webhook second" test didn't check BUTTON→CREDENTIAL or name	✅	`5ac58b0` asserts `open_trigger == "CREDENTIAL"` and, after `settle()`, `personName == "NOMBRE VERIFICADO"` (`:238-241`; stub seeded at `:221`).
P3 `summaryLabel` stays "Salida: Botón de Apertura / Sensor"; record on #73	✅	#73 comment 2048 (2026-09-25T09:43Z): "a card-only cycle can retain the Spanish button/sensor `summaryLabel` after async person-name resolution … derive that label from the final trigger/name in the active UI language."

New findings

P3 tests/test_cardholder_name_resolution.py:243-252 (same gap as the partial row). Untested: an existing open cycle (open_door(..., already_open=True) + anonymous opening) followed by an alarm webhook carrying a card; and that open_longest_item(...)["open_trigger"] stays FORCED after settle() resolves the name. Spec: "Button / manual / forced / unknown triggers unaffected."
P3 #73 comment 2048 says the cycle has "CREDENTIAL trigger" after resolution; the write-back never changes the trigger — it's CREDENTIAL only because the card set it at ingestion (resolve_open_trigger with card_no). Wording only; a forced cycle stays FORCED. No PR action.
90d3841 also clears person_id in both sanitizer paths (sanitizer_repository.py:52-57, :111-116), guarded like card_no. Task-retention and type-guard fixes add no spec drift.

Whole-diff re-check: every settled-spec bullet still met ("carried onto cycle rows alongside cardNo", "resolves a blank name via person/personId/personInfo, cached onto the cycle", "No per-poll HikCentral request", no local-list match / 300 s window, "Frontend unchanged"). No P1/P2.

Merge readiness (this axis): Ready. Remaining P3 (FORCED on an already-open cycle) → follow-up issue.

Summary: Blocking (P2): Standards — person_id export masking untested (existing sanitizer test silently skips it). Spec ready (P3 only). Worst per axis: Standards → that untested masking; Spec → FORCED-on-existing-cycle path still untested (P3).

🤖 Generated with Claude Code

# Code review — pass 2 (two-axis) Re-reviewed at head `5ac58b0`. 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 Checked `90d3841` and `5ac58b0` at head `5ac58b0`. In a throwaway worktree, `tests/test_cardholder_name_resolution.py` + `tests/test_database_sync.py` pass (24), `ruff check` clean. Migration unchanged since pass 1 and still safe: checks columns before `ALTER ... ADD COLUMN person_id TEXT DEFAULT ''`, uses `CREATE INDEX IF NOT EXISTS` (`app/db/database.py:160-166`). ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | **P2** untracked `create_task` | ✅ fixed | `90d3841`, `person_directory.py:37,59-61`: tasks held in `self._tasks`, removed via `add_done_callback(self._tasks.discard)`; `_in_flight` still cleared in `finally` (`:71-72`). | | **P3** guard order in `fetch_name_async` | ✅ fixed | `90d3841`, `person_directory.py:85-92`: `isinstance(res, dict)` first. | | **P3** duplicated `person_id` extraction (`cycle_repository.py:101,160`; `door_service.py:292,372`) | ❌ not addressed | Unchanged. Judgement call → follow-up issue. | | **P3** Data Clumps / Shotgun Surgery (loose credential fields) | ❌ not addressed | Unchanged. Judgement call → follow-up issue. | | **P3** redundant `cardNo` assignment in `has_person` branch | ✅ fixed | `90d3841` removes it (`access_cycle_aggregator.py:86-95`). Side benefit: a named event with no card no longer wipes an existing `cardNo`. | | **P3** unbounded name cache undocumented | ✅ fixed | `person_directory.py:29` docstring: cached until restart. | | **P3** `person_id` not masked in exports | ✅ fixed in code, untested (see N1) | `sanitizer_repository.py:52-57,111-116` sets `person_id = ''` in sync and async paths. | | **P3** `fill_person_name_async` doesn't fill `person_role` | ⊘ declined with reason | "Review follow-up": person-info lookup yields a name; card-only lookup won't invent a role. | | **P3** no Checklist section | ✅ fixed | `## Checklist` with four ticked items. | ### New findings 1. **P2** `app/db/sanitizer_repository.py:52-57,111-116`: `person_id` masking has no test. code-standards §4.3: "Every new feature, endpoint, or service fix must have accompanying tests." The checklist ticks "Sanitized database copies clear person ids", but nothing checks it. The existing `tests/test_database_sync.py:95` seeds `door_access_cycles` without a `person_id` column, so the new `UPDATE` silently hits `OperationalError` (logged at debug) and the test still passes. A silent regression would leak ids into exported copies. Add `person_id` to the seeded table and assert `''` after sanitizing. 2. **P3** `app/db/cycle_repository.py:203-214`: `fill_person_name_async` has no guard for empty `person_id`. Today the only caller filters `''` (`person_directory.py:49`, `door_service.py:1396`). A future call with `''` would write one name onto every nameless cycle with no person id (~4,900 prod rows). Return 0 when empty. 3. **P3**, possible Duplicated Code: `sanitizer_repository.py` repeats the try/`UPDATE`/debug block in sync and async paths. Existing file pattern; note only. Nothing else at P1/P2. **Merge readiness (this axis):** One P2 left — the missing sanitizer test for `person_id` (a few lines). P3s → follow-up issue. ## Spec Reviewed `origin/master...origin/fix/telemetry-open-longest-cardholder` at head 5ac58b0, mainly fix commits 90d3841 and 5ac58b0 against #46's settled design comment. Throwaway worktree: `tests/test_cardholder_name_resolution.py` **8 passed**; full suite **290 passed, 1 skipped**. ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P3 "Verified across Spanish and English localizations" not evidenced | ⊘ declined-with-reason | Description "Review follow-up" and #73 comment 2048 ("This also covers English and Spanish verification for the cardholder-name flow") hand this to #73. Frontend unchanged; low risk. | | P3 FORCED exposed: alarm event with a card copies `cardNo`/`personId`; test FORCED wins | ◐ partial | 5ac58b0 adds `test_forced_open_with_card_keeps_forced_trigger` (`tests/test_cardholder_name_resolution.py:243-252`), asserting `cardNo`, `personId`, `open_trigger == "FORCED"`. But the door starts closed, so the event takes the **create** path (`access_cycle_aggregator.py:51`). The copy lines the finding pointed at (`:84-87`, `else` branch, existing cycle) remain unexercised. Reading the code, FORCED still wins there (`:101-104` sets FORCED when `personName` is blank), but no test pins it. | | P3 retry pause and unbounded `_names` cache | ✅ | 90d3841 docstring: "Resolved names remain cached until process restart" (`person_directory.py:29`). No size limit, accepted in pass 1. | | P3 "Poll first, card webhook second" test didn't check BUTTON→CREDENTIAL or name | ✅ | 5ac58b0 asserts `open_trigger == "CREDENTIAL"` and, after `settle()`, `personName == "NOMBRE VERIFICADO"` (`:238-241`; stub seeded at `:221`). | | P3 `summaryLabel` stays "Salida: Botón de Apertura / Sensor"; record on #73 | ✅ | #73 comment 2048 (2026-09-25T09:43Z): "a card-only cycle can retain the Spanish button/sensor `summaryLabel` after async person-name resolution … derive that label from the final trigger/name in the active UI language." | ### New findings - **P3** `tests/test_cardholder_name_resolution.py:243-252` (same gap as the partial row). Untested: an existing open cycle (`open_door(..., already_open=True)` + anonymous opening) followed by an alarm webhook carrying a card; and that `open_longest_item(...)["open_trigger"]` stays `FORCED` after `settle()` resolves the name. Spec: "Button / manual / forced / unknown triggers unaffected." - **P3** #73 comment 2048 says the cycle has "CREDENTIAL trigger" after resolution; the write-back never changes the trigger — it's CREDENTIAL only because the card set it at ingestion (`resolve_open_trigger` with `card_no`). Wording only; a forced cycle stays FORCED. No PR action. - 90d3841 also clears `person_id` in both sanitizer paths (`sanitizer_repository.py:52-57`, `:111-116`), guarded like `card_no`. Task-retention and type-guard fixes add no spec drift. Whole-diff re-check: every settled-spec bullet still met ("carried onto cycle rows alongside `cardNo`", "resolves a blank name via `person/personId/personInfo`, cached onto the cycle", "No per-poll HikCentral request", no local-list match / 300 s window, "Frontend unchanged"). No P1/P2. **Merge readiness (this axis):** Ready. Remaining P3 (FORCED on an already-open cycle) → follow-up issue. --- **Summary:** **Blocking (P2):** Standards — `person_id` export masking untested (existing sanitizer test silently skips it). Spec ready (P3 only). Worst per axis: Standards → that untested masking; Spec → FORCED-on-existing-cycle path still untested (P3). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added 1 commit

2026-09-25 12:13:39 +00:00

test: cover person id masking in sanitized database copies

CI / lint-and-test (pull_request) Successful in 1m25s

Details

cfefbf335a

The sanitizer test seeded `door_access_cycles` without a `person_id`
column, so the new masking step failed silently and the test still
passed. Seed the column and assert both the async and sync paths clear it.

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

gabogg referenced this pull request

2026-09-25 12:14:47 +00:00

follow-up(telemetry): P3 cleanups from #93 review (cardholder name) #101

gabogg merged commit 132621345e into master

2026-09-25 12:17:46 +00:00

gabogg deleted branch fix/telemetry-open-longest-cardholder

2026-09-25 12:17:46 +00:00

gabogg referenced this pull request from a commit

2026-09-25 12:17:46 +00:00

fix(telemetry): show the cardholder name for card-opened doors (#46) (#93)