No reviewers
Labels
No labels
blocked
bug
enhancement
high-priority
low-priority
needs-info
needs-triage
ready-for-agent
ready-for-human
referenced
research
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gabogg/hikcentral!117
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/occupancy-review-followups-53-54"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Problem Statement
This PR resolves non-blocking follow-up findings from the reviews of PR #53 (#65) and PR #54 (#66):
is_unlisted=Truestate persisted indefinitely when a group disappeared from Artemiscount_listandgroup_list.lifespan: an exception inenforce_counting_integrity_asyncskipped initial count sync and anomaly reconciliation.OccupancyRepository.get_camera_asyncand raw dictionarycam_payload._group_skip_reasonsstate drifted from DB when groups disappeared or became listed.group_cam_mapstored raw dicts instead of a typed structure.GroupMemberandGroupSkipReasonlived in the service with sentence values rather than standard codes in schema.QuarantineTally.totalambiguously named; should bepassages.HTTP_422_UNPROCESSABLE_ENTITYvsHTTP_422_UNPROCESSABLE_CONTENT).not is_excluded and is_activein Python.get_hourly_flow_distribution_asyncended at<= cycle_end, whileget_bucketed_cycle_flow_asyncused< ?._insert_eventintest_trust_dataset_parity.pytook ambiguoushourand overridingatarguments.CONTEXT.mdfor Multi-camera group contradicted invariant definition ("does not allow for" vs "does not account for").+ 24*3600) that real callers never pass, creating a 25th bucket (hour_idx = 24).Architectural Approach
OccupancyManager._skip_group_async: persistis_unlistedviarepo.mark_group_unlisted_asyncbefore caching the reason inself._group_skip_reasons.OccupancyRepository: makemark_group_unlisted_asyncidempotent, and addclear_stale_unlisted_groups_asyncto clearis_unlisted=0for groups no longer reported.self._group_skip_reasonsfor codes not incount_list, and pop registered groups on camera group registration.app/main.py: lifespan, isolateenforce_counting_integrity_asyncin its owntry/exceptblock.GroupMember,GroupSkipReason(with short enum values andGROUP_SKIP_MESSAGES), andGroupDirectionMappingintoapp/schemas/occupancy_models.py.resource_group_codetoCountingCameraItem; makeget_camera_asyncreturnCountingCameraItem | Noneandupsert_camera_asynchandle both schema and dict.QuarantineTally.totaltopassageswith a docstring and backwards-compatible property/validator.status.HTTP_422_UNPROCESSABLE_CONTENTinapp/main.pyand pinstarlette>=0.37.2inrequirements.txt._COUNTED_CAMERA_JOIN/_COUNTED_CAMERA_FILTERcomment and use_is_counted_camerahelper for top entrance/exit ranking.get_bucketed_cycle_flow_asyncto use inclusive<= ?, and updateanalytics_service.pyto passbounds.end.CONTEXT.mdglossary wording to "does not account for".tests/test_trust_dataset_parity.py: useCYCLE_END = CYCLE_START + 24*3600 - 0.001, helper_hour_epoch(hour), and assert bucket indices 0 and 23.tests/test_counting_integrity.pyverifying unlisted write retry, stale unlisted state cleanup, and decoupled startup integrity checks.Verification Evidence
ruff check .: Passed cleanly with zero warnings/errors.ruff format --check .: Passed, all 129 files formatted.git diff --check: Passed, zero trailing whitespace or formatting defects.node --test: 80 passed in 1.0s.pytest -q -o faulthandler_timeout=30: 336 passed, 0 failed, 1 skipped in 66s.Closes #65
Closes #66
Two-Axis Code Review (PR #117)
Standards
1. Documented Standard Violations (Hard Violations)
OccupancyRepository.upsert_camera_async(app/db/occupancy_repository.py):docs/standards/code-standards.md§2.3 (Pydantic Schemas: "Avoid passing untyped, raw dictionaries between service layers when structured schemas are available").CountingCameraItem | dict[str, Any]and returns untypeddict[str, Any]. Inapp/services/occupancy_service.py:477, callers also pass raw dict{"camera_index_code": ec_code, "is_active": False}, bypassing strict model validation.GroupMemberandGroupDirectionMapping(app/schemas/occupancy_models.py):docs/standards/code-standards.md§2.3 ("Use Pydantic v2 schemas in app/schemas/ for all API request bodies and structured data payloads") andAGENTS.md§2.typing.NamedTuplerather than Pydantic v2BaseModelclasses, breaking schema modeling consistency inapp/schemas/.2. Baseline Smells (Judgement Calls)
Feature Envy & Primitive Obsession —
app/db/occupancy_repository.py:Assessment: The helper envies camera attributes via untyped dictionary lookups (
dict[str, Any]). This predicate belongs as anis_countedproperty directly onCountingCameraItem.Primitive Obsession —
OccupancyRepository.upsert_camera_async(app/db/occupancy_repository.py):Assessment: Degrades typed Pydantic models back into raw
dict[str, Any]and returns a dictionary instead of maintainingCountingCameraItem.Primitive Obsession —
tests/test_trust_dataset_parity.py:Assessment: Hardcodes the magic float
0.001instead of referencing the domain constantCYCLE_END_EPSILONfromapp.facility_time.Spec
(a) Missing or Partial Requirements
Issue #66, Item 2:
Finding: Partial caller migration. While
OccupancyRepository.get_bucketed_cycle_flow_asyncwas updated from< ?to<= ?andAnalyticsService.get_business_cycle_breakdown_asyncwas switched to passbounds.end,AnalyticsService.get_cycle_timeseries_asyncwas omitted and still passesend_epoch = cycle.next_start.(b) Behaviour in Diff Not Asked For (Scope Creep)
Issue #65, Item 3:
Finding: In
app/main.py,occupancy_manager.sync_cameras_from_artemis_async()was also wrapped in its own dedicatedtry/exceptblock. The spec only asked to decoupleenforce_counting_integrity_asyncso counts sync and anomaly reconciliation wouldn't abort if integrity enforcement failed.(c) Requirements Implemented Where Implementation Looks Wrong
Issue #66, Item 2:
Finding: Because
OccupancyRepository.get_bucketed_cycle_flow_asyncnow evaluatestimestamp_epoch <= ?, the un-migrated callerAnalyticsService.get_cycle_timeseries_asyncqueries the closed interval[cycle.start, cycle.next_start]. An event stamped exactly atcycle.next_start(which belongs to the subsequent cycle) matches in SQL withbucket_idx = 24. Althoughget_cycle_timeseries_asyncsilently discards it via0 <= idx < num_buckets, the query improperly spans across cycle boundaries.Summary: 5 findings in Standards (worst:
upsert_camera_asyncaccepting and returning untyped dictionaries in violation of §2.3); 3 findings in Spec (worst: un-migrated callerAnalyticsService.get_cycle_timeseries_asyncmatching across cycle boundaries under the new inclusive<= ?window convention).Addressed Review Findings (Commit
203696d)All findings identified in the two-axis code review have been addressed:
Standards — Pydantic Schemas & Typing:
GroupMemberandGroupDirectionMappingfromNamedTupleto Pydantic v2BaseModelclasses inapp/schemas/occupancy_models.py.OccupancyRepository.upsert_camera_asyncsignature and return value to returnCountingCameraItem.CountingCameraIteminstances when deactivating obsolete cameras inOccupancyManager.sync_cameras_from_artemis_async.Standards — Smells & Domain Consistency:
is_countedproperty directly onCountingCameraItemand updated_is_counted_camerato delegate to it.0.001intests/test_trust_dataset_parity.pywith domain constantCYCLE_END_EPSILONfromapp.facility_time.Spec — Window-End Convention Parity:
AnalyticsService.get_cycle_timeseries_asyncto query[cycle.start, cycle.end]using the inclusive<= ?convention while sizing bucket arrays usingcycle.next_start, preventing boundary event leakage into the 25th bucket.test_window_bounds_match_at_the_cycle_edgesintests/test_trust_dataset_parity.pyto verify that bothget_hourly_flow_distribution_asyncandget_bucketed_cycle_flow_asyncmatch identical bucket distributions and exclude events atcycle.next_start.All checks, linters (
ruff), and full test suites (pytest336 passed,node --test80 passed) are green.