feat(occupancy): retrieve and stage historical HikCentral flow #166

Merged

gabogg merged 6 commits from feat/historical-flow-backfill into master 2026-09-28 11:01:01 +00:00

Conversation 5 Commits 6 Files changed 17 +2306 -30

gabogg commented 2026-09-27 23:39:54 +00:00

Owner

Copy link

Summary

Local passenger-flow history begins on 2026-09-03, leaving older days unavailable for closed-month and year-over-year statistics. The deployed HikCentral OpenAPI 2.6.3 service returns hourly IN/OUT counts per counting camera from before that date (verified for July and August 2026).

This PR delivers the first step of the historical-flow RFC: retrieval and durable, revisioned staging (#162).

• An admin can fetch a completed facility-calendar month from HikCentral into private Historical Flow Staging.
• The fetch stores per-request evidence and per-camera-hour counts with month-scoped revisions.
• Camera count and identity mismatches are flagged for review.
• Nothing reaches live events, calibration or statistics.

Scope change (2026-09-28): this PR was first planned as the single integration branch for #162–#165. It now closes #162 only; the rest moved to stacked drafts (see the split comment below):

Issue	Draft PR	Base
#163 monthly curation	#170	this branch
#164 publication and calibration exclusion	#171	#170
#165 statistics and deck	#172	#171

Merging #162 first allows a real read-only fetch on production. That settles what the RFC leaves open (completeness meaning, empty-hour behavior, retention depth) before curation is designed on top. #160 stays open as the umbrella until #165 lands.

Architectural impact

• Client: ArtemisClient.people_counting_by_hour_async for statisticsTotalNumByTime (at most 10 comma-joined cameras, hourly, ISO times with facility offset).
• Tables: flow_history_months, flow_history_fetches, flow_history_requests, flow_history_camera_hours.
  • Rows are insert-only and keyed by (month, source, camera, hour, revision).
  • The 180-day live-event pruning does not touch these tables.
  • Nothing is written to people_counting_events.
• Service: FlowHistoryService runs background fetches, one at a time per month.
  • Each request covers one business day for one batch of at most 10 cameras.
  • A range pages until a page adds no new key.
  • Rejected rows are counted, and the lowest completeness is kept.
  • Fetches cut short by a restart are marked interrupted at startup.
• Month summary: evidence per day and camera, based on each one's most recent settled attempt.
• Routes: admin-only /api/occupancy/history/… (POST /fetches, GET /fetches, GET /fetches/{id}, GET /months/{month}).
• Shared code:
  • A new app/controllers/errors.domain_errors replaces the duplicated error mapping in the statistics, analytics and history controllers.
  • New in app/facility_time: FacilityMonth and configured_reset_time.

Verification

• python3 scripts/check_docs.py
• ruff check . and ruff format --check .
• Full pytest suite on master + PR: 450 passed (93df647). Earlier run: 439 passed. A pre-commit re-run on b07a0b9 passed, including the 5 leap-year cases added afterwards.
• Review pass 1 posted, and all P1/P2/P3 findings fixed in b07a0b9.
• Review pass 2 posted; all findings (3 P2, 1 P3) fixed in 93df647.

Checklist

• Publish the agreed RFC and glossary terms.
• Decompose #160 into child issues.
• Implement #162 (retrieval and staging).
• Split #163–#165 into stacked draft PRs #170–#172.
• Review pass 2 and its fixes.
• After merge: a read-only production fetch of one completed month, with the findings recorded on #160/#163.

Issues closed on merge

Closes #162

Part of #160 (umbrella; stays open until #165 lands).

🤖 Generated with Claude Code

## Summary Local passenger-flow history begins on 2026-09-03, leaving older days unavailable for closed-month and year-over-year statistics. The deployed HikCentral OpenAPI 2.6.3 service returns hourly IN/OUT counts per counting camera from before that date (verified for July and August 2026). This PR delivers the first step of the [historical-flow RFC](https://git.gaboggamer.online/gabogg/hikcentral/src/branch/feat/historical-flow-backfill/docs/architecture/rfc-historical-flow-backfill.md): **retrieval and durable, revisioned staging (#162)**. - An admin can fetch a completed facility-calendar month from HikCentral into private Historical Flow Staging. - The fetch stores per-request evidence and per-camera-hour counts with month-scoped revisions. - Camera count and identity mismatches are flagged for review. - Nothing reaches live events, calibration or statistics. **Scope change (2026-09-28):** this PR was first planned as the single integration branch for #162–#165. It now closes #162 only; the rest moved to stacked drafts (see the split comment below): | Issue | Draft PR | Base | | --- | --- | --- | | #163 monthly curation | #170 | this branch | | #164 publication and calibration exclusion | #171 | #170 | | #165 statistics and deck | #172 | #171 | Merging #162 first allows a real read-only fetch on production. That settles what the RFC leaves open (`completeness` meaning, empty-hour behavior, retention depth) before curation is designed on top. #160 stays open as the umbrella until #165 lands. ## Architectural impact - **Client:** `ArtemisClient.people_counting_by_hour_async` for `statisticsTotalNumByTime` (at most 10 comma-joined cameras, hourly, ISO times with facility offset). - **Tables:** `flow_history_months`, `flow_history_fetches`, `flow_history_requests`, `flow_history_camera_hours`. - Rows are insert-only and keyed by `(month, source, camera, hour, revision)`. - The 180-day live-event pruning does not touch these tables. - Nothing is written to `people_counting_events`. - **Service:** `FlowHistoryService` runs background fetches, one at a time per month. - Each request covers one business day for one batch of at most 10 cameras. - A range pages until a page adds no new key. - Rejected rows are counted, and the lowest `completeness` is kept. - Fetches cut short by a restart are marked interrupted at startup. - **Month summary:** evidence per day and camera, based on each one's most recent settled attempt. - **Routes:** admin-only `/api/occupancy/history/…` (`POST /fetches`, `GET /fetches`, `GET /fetches/{id}`, `GET /months/{month}`). - **Shared code:** - A new `app/controllers/errors.domain_errors` replaces the duplicated error mapping in the statistics, analytics and history controllers. - New in `app/facility_time`: `FacilityMonth` and `configured_reset_time`. ## Verification - [x] `python3 scripts/check_docs.py` - [x] `ruff check .` and `ruff format --check .` - [x] Full pytest suite on master + PR: 450 passed (`93df647`). Earlier run: 439 passed. A pre-commit re-run on `b07a0b9` passed, including the 5 leap-year cases added afterwards. - [x] Review pass 1 posted, and all P1/P2/P3 findings fixed in `b07a0b9`. - [x] Review pass 2 posted; all findings (3 P2, 1 P3) fixed in `93df647`. ## Checklist - [x] Publish the agreed RFC and glossary terms. - [x] Decompose #160 into child issues. - [x] Implement #162 (retrieval and staging). - [x] Split #163–#165 into stacked draft PRs #170–#172. - [x] Review pass 2 and its fixes. - [ ] After merge: a read-only production fetch of one completed month, with the findings recorded on #160/#163. ## Issues closed on merge Closes #162 Part of #160 (umbrella; stays open until #165 lands). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added 1 commit 2026-09-27 23:39:54 +00:00

docs(occupancy): specify reviewed historical flow backfill 32484fbbd2

gabogg referenced this pull request 2026-09-27 23:40:08 +00:00

feat(occupancy): reviewed monthly historical passenger-flow backfill #160

gabogg added the

ready-for-agent

high-priority

labels 2026-09-28 00:07:24 +00:00

gabogg added 1 commit 2026-09-28 00:34:23 +00:00

Merge remote-tracking branch 'origin/master' into feat/historical-flow-backfill a236666cba

gabogg added 1 commit 2026-09-28 08:31:00 +00:00

feat(occupancy): retrieve and stage historical HikCentral flow (#162) ... fec62f32c9

Admins can fetch a completed facility-calendar month of HikCentral hourly
people counts into durable Historical Flow Staging without touching live
events, calibration or statistics.

- Artemis statisticsTotalNumByTime client: <=10 comma-joined cameras,
  hourly statisticsType, ISO times with facility offset.
- One bounded request per business day and camera batch; endTime stops one
  second before the next range. Paging continues until a page adds no new
  key (capped), since the deployed service ignores pageSize and repeats
  pages. Out-of-range, off-hour, foreign-camera and invalid rows are
  rejected; conflicting duplicates fail the range.
- Staging tables for fetch runs, per-request evidence (status, API code,
  completeness, pages, row counts) and camera-hour rows. New or changed
  rows take the month's next revision; unchanged re-fetches are idempotent
  and prior snapshots stay readable. Absent rows are never read as zero.
- Background fetch with one running fetch per month; shutdown/restart
  marks it interrupted and keeps retrieved rows for safe retry. Camera
  count/identity mismatches are flagged for review.
- Admin-only routes under /api/occupancy/history; API inventory and RFC
  updated.

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

gabogg changed title from WIP: feat(occupancy): reviewed historical passenger-flow backfill to feat(occupancy): reviewed historical passenger-flow backfill 2026-09-28 08:32:54 +00:00

gabogg commented 2026-09-28 08:33:30 +00:00

Author

Owner

Copy link

Review pass 1 — fec62f3 (#162 retrieval and staging)

Two-axis review of a236666...fec62f3: Standards (AGENTS.md, docs/standards/code-standards.md, and the Fowler smell baseline) and Spec (#162 and the RFC as of a236666). This is the first pass, so every P1, P2 and P3 finding will be addressed on this branch before a second pass is requested.

Spec

• P1: a revision collision across months and inside one fetch overwrites staged snapshots.
  • Revisions are counted per month. However, the latest-value lookup, the MAX(revision) snapshot subquery and UNIQUE(source, camera, hour_epoch, revision) all ignore month.
  • With a reset that is not on the hour (e.g. 04:30), the straddling hour on the 1st is fetched by both months. Month M+1's revision-1 insert then takes the ON CONFLICT … DO UPDATE path and overwrites month M's row in place, leaving stale month and fetch_id values.
  • The same upsert silently overwrites within one fetch when two day ranges return different counts for the shared hour.
  • Spec: "Deduplicate by source, camera and hour"; "Retain prior snapshots when a re-fetch changes counts".
• P2: camera count/identity validation can be bypassed.
  • When the admin passes an explicit list, expected = request.expected_camera_count or len(codes), so the count check never fires.
  • unregistered_camera_codes does not set camera_mismatch.
  • Registered active cameras left out of an explicit list are never flagged.
  • Spec: "Default to the current count of 12 cameras … surface and require validation of a count or identity mismatch".
• P2: the upstream completeness signal is only half-captured.
  • It is kept only when it is an int, and only from the last page.
  • It never reaches the fetch or the per-day evidence.
  • Spec: "report generation/availability and gaps remain reviewable".
• P2: the month summary counts failed ranges from the latest fetch only.
  • A narrower later fetch, or one still running, hides earlier failed ranges.
  • Spec: "gaps remain reviewable".
• P2: stale running fetches after a restart.
  • They are marked interrupted only when a new fetch for the same month starts, so the list shows them as running indefinitely.
• P2: several tests do not prove what they claim.
  • The partial-failure cameras_with_missing_hours == CAMERAS assertion is trivially true.
  • The "inclusive boundary row" never reaches a range, so range rejection and cross-range dedup go unexercised.
  • No test uses a reset that is not on the hour.
  • "No changes to live/published data" is checked on only two tables.
• P3: possible scope creep. GET /months/{month} per-day coverage leans toward #163 day review. It is defensible as coverage evidence.

Standards

• P2: raw dicts between layers. list_month_requests_async -> list[dict[str, Any]] is consumed by key in the service. Standard: code-standards §2.3, "Avoid passing untyped, raw dictionaries between service layers when structured schemas are available". Also by_day: dict[date, list[Any]].
• P3: weak typing where a Literal exists. RequestEvidence.status: str and the untyped status local in _run_fetch should use RequestStatus/FetchStatus.
• P3: generic error code. A new generic CONFLICT code was added. AGENTS.md §2 asks for established or specific codes.
• P3 (Duplicated Code):
  • handle_controller_errors copies the statistics controller's version.
  • cfg.get("daily_reset_time") or "04:00" appears twice.
  • The (enter_num, exit_num) comparison appears in both the repository and the service.
• P3 (Data Clumps): RequestEvidence mirrors HistoricalFlowRequestRecord field for field.
• P3 (Feature Envy): the service filters month requests by fetch and status in Python. That filter belongs in the repository query.
• P3 (Mysterious Name): _fetch_revision reads as a verb. parse_month() is called only for its validation side effect.
• P3 (long method): _fetch_range is about 85 lines with 11 mutable locals.
• P3 (Primitive Obsession): the month travels as a regex-validated str and is re-parsed at several sites.
• P3: wait_for_fetch_async is only used by tests, which widens the service's surface.
• P3: assert fetch is not None is used as control flow in the repository. It is stripped under -O.

Totals: Spec: 1 P1, 5 P2, 1 P3. Standards: 1 P2, about 10 P3. Worst per axis: the Spec P1 revision/snapshot overwrite, and the Standards P2 raw dicts crossing the repository→service seam.

## Review pass 1 — `fec62f3` (#162 retrieval and staging) Two-axis review of `a236666...fec62f3`: **Standards** (AGENTS.md, `docs/standards/code-standards.md`, and the Fowler smell baseline) and **Spec** (#162 and the RFC as of `a236666`). This is the first pass, so every P1, P2 and P3 finding will be addressed on this branch before a second pass is requested. ### Spec - **P1: a revision collision across months and inside one fetch overwrites staged snapshots.** - Revisions are counted per month. However, the latest-value lookup, the `MAX(revision)` snapshot subquery and `UNIQUE(source, camera, hour_epoch, revision)` all ignore `month`. - With a reset that is not on the hour (e.g. 04:30), the straddling hour on the 1st is fetched by both months. Month M+1's revision-1 insert then takes the `ON CONFLICT … DO UPDATE` path and overwrites month M's row in place, leaving stale `month` and `fetch_id` values. - The same upsert silently overwrites within one fetch when two day ranges return different counts for the shared hour. - Spec: "Deduplicate by source, camera and hour"; "Retain prior snapshots when a re-fetch changes counts". - **P2: camera count/identity validation can be bypassed.** - When the admin passes an explicit list, `expected = request.expected_camera_count or len(codes)`, so the count check never fires. - `unregistered_camera_codes` does not set `camera_mismatch`. - Registered active cameras left out of an explicit list are never flagged. - Spec: "Default to the current **count** of 12 cameras … surface and require validation of a count or identity mismatch". - **P2: the upstream `completeness` signal is only half-captured.** - It is kept only when it is an `int`, and only from the last page. - It never reaches the fetch or the per-day evidence. - Spec: "report generation/availability and gaps remain reviewable". - **P2: the month summary counts failed ranges from the latest fetch only.** - A narrower later fetch, or one still running, hides earlier failed ranges. - Spec: "gaps remain reviewable". - **P2: stale `running` fetches after a restart.** - They are marked `interrupted` only when a new fetch for the same month starts, so the list shows them as running indefinitely. - **P2: several tests do not prove what they claim.** - The partial-failure `cameras_with_missing_hours == CAMERAS` assertion is trivially true. - The "inclusive boundary row" never reaches a range, so range rejection and cross-range dedup go unexercised. - No test uses a reset that is not on the hour. - "No changes to live/published data" is checked on only two tables. - **P3: possible scope creep.** `GET /months/{month}` per-day coverage leans toward #163 day review. It is defensible as coverage evidence. ### Standards - **P2: raw dicts between layers.** `list_month_requests_async -> list[dict[str, Any]]` is consumed by key in the service. Standard: code-standards §2.3, "Avoid passing untyped, raw dictionaries between service layers when structured schemas are available". Also `by_day: dict[date, list[Any]]`. - **P3: weak typing where a Literal exists.** `RequestEvidence.status: str` and the untyped `status` local in `_run_fetch` should use `RequestStatus`/`FetchStatus`. - **P3: generic error code.** A new generic `CONFLICT` code was added. AGENTS.md §2 asks for established or specific codes. - **P3 (Duplicated Code):** - `handle_controller_errors` copies the statistics controller's version. - `cfg.get("daily_reset_time") or "04:00"` appears twice. - The `(enter_num, exit_num)` comparison appears in both the repository and the service. - **P3 (Data Clumps):** `RequestEvidence` mirrors `HistoricalFlowRequestRecord` field for field. - **P3 (Feature Envy):** the service filters month requests by fetch and status in Python. That filter belongs in the repository query. - **P3 (Mysterious Name):** `_fetch_revision` reads as a verb. `parse_month()` is called only for its validation side effect. - **P3 (long method):** `_fetch_range` is about 85 lines with 11 mutable locals. - **P3 (Primitive Obsession):** the month travels as a regex-validated `str` and is re-parsed at several sites. - **P3:** `wait_for_fetch_async` is only used by tests, which widens the service's surface. - **P3:** `assert fetch is not None` is used as control flow in the repository. It is stripped under `-O`. **Totals:** Spec: 1 P1, 5 P2, 1 P3. Standards: 1 P2, about 10 P3. Worst per axis: the Spec P1 revision/snapshot overwrite, and the Standards P2 raw dicts crossing the repository→service seam.

gabogg added 1 commit 2026-09-28 08:49:04 +00:00

fix(occupancy): address review pass 1 on historical flow staging (#162) ... b07a0b9ec3

Spec:
- P1: scope staging keys by month (UNIQUE and snapshot lookups include
  month) and drop the in-place upsert, so an hour straddling an off-the-hour
  reset is staged by each month without overwriting the other. A straddling
  hour whose counts differ between two days of one fetch fails the later
  range instead of silently replacing the first.
- Expected camera count defaults to the current count of active cameras even
  for explicit lists; unregistered and omitted cameras, cameras without rows
  and unexpected codes are recorded as camera_mismatch_reasons.
- Keep the lowest completeness across pages (numeric strings accepted) and
  report it per business day.
- Month summary evidence uses each day and camera's most recent attempt
  among settled fetches, so a narrower or running fetch hides nothing.
- Mark fetches left running as interrupted at startup.
- Tests: real boundary leakage, fully staged partial-failure day, 04:30
  reset, narrower re-fetch, completeness, camera expectations, startup
  recovery, all non-staging tables unchanged, calendar month lengths
  including leap years.

Standards:
- Typed request records from the repository (no raw dicts); settled-fetch
  filter in SQL; RequestEvidence folded into a shared Pydantic base.
- Shared controllers.errors.domain_errors replaces duplicated
  handle_controller_errors (statistics, analytics, history); a running
  fetch returns HISTORY_FETCH_RUNNING.
- FacilityMonth value type and configured_reset_time in facility_time.
- Split _fetch_range into page harvesting helpers; Literal-typed statuses;
  explicit LookupError instead of asserts; clearer names.

Also restores the RFC "Storage and monthly review" heading lost in the
previous commit.

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

gabogg commented 2026-09-28 08:49:24 +00:00

Author

Owner

Copy link

Review pass 1: resolution in b07a0b9

Every pass-1 finding has been addressed; details below. Full suite green: 439 tests in the last full run, plus 5 leap-year cases added afterwards, and the pre-commit pytest re-ran on commit. Ruff and check_docs.py are clean.

Spec

• P1: revision collision.
  • Staging keys now include the month: UNIQUE(month, source, camera, hour, revision), and the latest-value lookup and the snapshot subquery both filter on month.
  • The ON CONFLICT DO UPDATE is gone, so staged rows are insert-only.
  • A straddling hour is staged by each month independently.
  • Within one fetch, if the two days sharing a straddling hour return different counts, the later range fails. A repository backstop raises StagedRevisionConflictError if a same-revision overwrite is ever attempted.
  • Tests: test_straddling_hour_is_staged_by_each_month_without_overwriting, test_straddling_hour_disagreeing_between_days_fails_the_later_range.
• P2: camera validation.
  • The expected count defaults to the current count of active counting cameras, even when an explicit list is given.
  • New omitted_camera_codes field.
  • camera_mismatch_reasons lists count, unregistered, omitted, without-rows and unexpected differences, and camera_mismatch is derived from it.
  • The RFC now states that #163 approval must require confirming these reasons.
• P2: completeness.
  • The lowest value across pages is kept; numeric strings are accepted.
  • The day summary reports completeness_values and completeness_unreported.
• P2: summary from the latest fetch only.
  • Evidence is now the most recent attempt per business day and camera, among settled (non-running) fetches, fetched with the filter in SQL.
  • New cameras_failed and cameras_never_requested fields.
• P2: stale running fetches. recover_interrupted_async() runs at startup.
• P2: tests.
  • Real boundary leakage (the fake returns the next range's first hour).
  • A fully staged day for the partial-failure case.
  • A narrower re-fetch does not hide failures.
  • A 04:30 reset.
  • Completeness handling.
  • Camera expectations.
  • Startup recovery.
  • Every non-staging table is unchanged after a fetch.
  • Calendar month lengths, including leap years (2028-02, 2000-02) and century non-leap years (2100-02).
• P3: day coverage in GET /months/{month}. Kept as coverage evidence ("gaps remain reviewable"). The schema and docstring state it is not a Complete/Partial/Missing verdict; #163 owns the verdicts.

Standards

• P2: raw dicts. The repository returns HistoricalFlowRequestRecord, and by_day became a typed range slice.
• P3: typing. RequestStatus/FetchStatus are used for the evidence and the run status.
• P3: error code. A running fetch now returns HISTORY_FETCH_RUNNING (409).
• P3: duplication.
  • New shared app/controllers/errors.domain_errors, used by the statistics, analytics and history controllers.
  • configured_reset_time() lives in facility_time.
  • CameraHourCount.counts/.key replace the repeated tuple comparisons.
• P3: data clump. New HistoricalFlowRequestEvidence base, which HistoricalFlowRequestRecord extends; the repository dataclass is removed.
• P3: feature envy. The settled-fetch filter is now in SQL.
• P3: names. _fetch_revision is now _revision_of_fetch, and the side-effect parse_month call is gone.
• P3: long method. _fetch_range is split into _harvest_pages, _page_items, _accept_page and _accept_row.
• P3: primitive obsession. New FacilityMonth value type (parse, business days, completion check).
• P3: wait_for_fetch_async. Kept as the service's completion handle for its background task, and documented as such. Tests use it instead of polling or mocking.
• P3: production asserts. Replaced by require_fetch_async raising LookupError.

Also restored the RFC "Storage and monthly review" heading, which fec62f3 dropped.

Ready for review pass 2.

## Review pass 1: resolution in `b07a0b9` Every pass-1 finding has been addressed; details below. Full suite green: 439 tests in the last full run, plus 5 leap-year cases added afterwards, and the pre-commit pytest re-ran on commit. Ruff and `check_docs.py` are clean. ### Spec - **P1: revision collision.** - Staging keys now include the month: `UNIQUE(month, source, camera, hour, revision)`, and the latest-value lookup and the snapshot subquery both filter on month. - The `ON CONFLICT DO UPDATE` is gone, so staged rows are insert-only. - A straddling hour is staged by each month independently. - Within one fetch, if the two days sharing a straddling hour return different counts, the later range fails. A repository backstop raises `StagedRevisionConflictError` if a same-revision overwrite is ever attempted. - Tests: `test_straddling_hour_is_staged_by_each_month_without_overwriting`, `test_straddling_hour_disagreeing_between_days_fails_the_later_range`. - **P2: camera validation.** - The expected count defaults to the current count of active counting cameras, even when an explicit list is given. - New `omitted_camera_codes` field. - `camera_mismatch_reasons` lists count, unregistered, omitted, without-rows and unexpected differences, and `camera_mismatch` is derived from it. - The RFC now states that #163 approval must require confirming these reasons. - **P2: completeness.** - The lowest value across pages is kept; numeric strings are accepted. - The day summary reports `completeness_values` and `completeness_unreported`. - **P2: summary from the latest fetch only.** - Evidence is now the most recent attempt per business day and camera, among settled (non-running) fetches, fetched with the filter in SQL. - New `cameras_failed` and `cameras_never_requested` fields. - **P2: stale running fetches.** `recover_interrupted_async()` runs at startup. - **P2: tests.** - Real boundary leakage (the fake returns the next range's first hour). - A fully staged day for the partial-failure case. - A narrower re-fetch does not hide failures. - A 04:30 reset. - Completeness handling. - Camera expectations. - Startup recovery. - Every non-staging table is unchanged after a fetch. - Calendar month lengths, including leap years (2028-02, 2000-02) and century non-leap years (2100-02). - **P3: day coverage in `GET /months/{month}`.** Kept as coverage evidence ("gaps remain reviewable"). The schema and docstring state it is not a Complete/Partial/Missing verdict; #163 owns the verdicts. ### Standards - **P2: raw dicts.** The repository returns `HistoricalFlowRequestRecord`, and `by_day` became a typed range slice. - **P3: typing.** `RequestStatus`/`FetchStatus` are used for the evidence and the run status. - **P3: error code.** A running fetch now returns `HISTORY_FETCH_RUNNING` (409). - **P3: duplication.** - New shared `app/controllers/errors.domain_errors`, used by the statistics, analytics and history controllers. - `configured_reset_time()` lives in `facility_time`. - `CameraHourCount.counts`/`.key` replace the repeated tuple comparisons. - **P3: data clump.** New `HistoricalFlowRequestEvidence` base, which `HistoricalFlowRequestRecord` extends; the repository dataclass is removed. - **P3: feature envy.** The settled-fetch filter is now in SQL. - **P3: names.** `_fetch_revision` is now `_revision_of_fetch`, and the side-effect `parse_month` call is gone. - **P3: long method.** `_fetch_range` is split into `_harvest_pages`, `_page_items`, `_accept_page` and `_accept_row`. - **P3: primitive obsession.** New `FacilityMonth` value type (parse, business days, completion check). - **P3: `wait_for_fetch_async`.** Kept as the service's completion handle for its background task, and documented as such. Tests use it instead of polling or mocking. - **P3: production asserts.** Replaced by `require_fetch_async` raising `LookupError`. Also restored the RFC "Storage and monthly review" heading, which `fec62f3` dropped. Ready for review pass 2.

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

feat(occupancy): curate monthly historical flow drafts #170

gabogg referenced this pull request 2026-09-28 09:01:01 +00:00

feat(occupancy): publish reviewed historical flow without calibrating #171

gabogg referenced this pull request 2026-09-28 09:01:05 +00:00

feat(statistics): present approved historical flow and baselines #172

gabogg changed title from feat(occupancy): reviewed historical passenger-flow backfill to feat(occupancy): retrieve and stage historical HikCentral flow 2026-09-28 09:01:37 +00:00

gabogg commented 2026-09-28 09:02:10 +00:00

Author

Owner

Copy link

Scope split: this PR now closes #162 only

At the maintainer's request (2026-09-28), this PR no longer closes the umbrella #160. It delivers retrieval and staging (#162), which is implemented and passed review pass 1. The remaining children moved to stacked draft PRs, each labelled high-priority:

Issue	Draft PR	Stacked on	Triage
#163 monthly curation workspace	#170	this branch	needs-triage: maintainer decision needed
#164 atomic publication and calibration exclusion	#171	#170	ready-for-agent
#165 statistics and deck	#172	#171	ready-for-agent

Why split

• #162 is safe to ship alone: staging is admin-only and never read by statistics or calibration.
• Once merged, a real read-only fetch of one completed month on production can settle what the RFC leaves open (completeness meaning, empty-hour behavior, retention depth) before #163's review model is designed on top of it.
• Smaller PRs keep review passes tractable.

Why #163 needs input: it reuses #113's per-day schedule records, #114's effective-date policy and #161's holiday/event identity. All three are open and unimplemented. #170 lists the options: private draft context now, wait for #113/#161, or split #163 so the server-side day review comes first.

What changed on this PR

• The title now names #162's scope.
• The body says Closes #162, and #160 is referenced as the umbrella without being auto-closed.
• #160 stays open until #165 lands.
• Code is unchanged. Review pass 2 is still required before merge.

## Scope split: this PR now closes #162 only At the maintainer's request (2026-09-28), this PR no longer closes the umbrella #160. It delivers retrieval and staging (#162), which is implemented and passed review pass 1. The remaining children moved to stacked draft PRs, each labelled `high-priority`: | Issue | Draft PR | Stacked on | Triage | | --- | --- | --- | --- | | #163 monthly curation workspace | #170 | this branch | `needs-triage`: maintainer decision needed | | #164 atomic publication and calibration exclusion | #171 | #170 | `ready-for-agent` | | #165 statistics and deck | #172 | #171 | `ready-for-agent` | **Why split** - #162 is safe to ship alone: staging is admin-only and never read by statistics or calibration. - Once merged, a real read-only fetch of one completed month on production can settle what the RFC leaves open (`completeness` meaning, empty-hour behavior, retention depth) before #163's review model is designed on top of it. - Smaller PRs keep review passes tractable. **Why #163 needs input:** it reuses #113's per-day schedule records, #114's effective-date policy and #161's holiday/event identity. All three are open and unimplemented. #170 lists the options: private draft context now, wait for #113/#161, or split #163 so the server-side day review comes first. **What changed on this PR** - The title now names #162's scope. - The body says `Closes #162`, and #160 is referenced as the umbrella without being auto-closed. - #160 stays open until #165 lands. - Code is unchanged. Review pass 2 is still required before merge.

gabogg referenced this pull request 2026-09-28 09:02:11 +00:00

feat(occupancy): reviewed monthly historical passenger-flow backfill #160

gabogg referenced this pull request 2026-09-28 10:28:34 +00:00

feat(occupancy): curate monthly historical flow drafts #163

gabogg added this to the Historical passenger-flow backfill milestone 2026-09-28 10:31:37 +00:00

gabogg commented 2026-09-28 10:32:05 +00:00

Author

Owner

Copy link

Review pass 2 — b07a0b9 (#162 retrieval and staging)

Reviewed a236666...b07a0b9, including the pass-1 fixes in fec62f3..b07a0b9, against issue #162, the historical-flow RFC, AGENTS.md, and the code standards. The pass-1 fixes addressed the previously reported collision, camera, completeness, recovery, and test concerns. This pass found the following remaining issues.

Standards

• P2 — Artemis response parsing crosses the client boundary. app/services/flow_history_service.py (_page_items, _accept_row, around lines 425–487) reads the vendor envelope and fields code, list, cameraIndexCode, enterNum, and exitNum. docs/standards/code-standards.md §1.1 says clients must isolate third-party protocol details so services receive domain-oriented inputs and outputs. Move envelope and row decoding into ArtemisClient or a client-side adapter, returning typed page/evidence data.
• P3 — Possible Duplicated Code smell (judgement call). app/facility_time.py and app/schemas/flow_history.py separately define the same YYYY-MM validation pattern. Sharing the pattern would keep month validation consistent across the domain and API schema.

Spec

• P2 — Day evidence changes if the facility reset time changes. get_month_summary_async builds day ranges with the current daily_reset_time (app/services/flow_history_service.py, around lines 507–532), while fetches retain their original request ranges. Changing the reset can move staged hours between days and report a recorded request as cameras_never_requested. The RFC requires “Every request's range ... [to be] stored with the fetch” and describes the month summary as per-business-day review evidence. Derive historical coverage from the persisted fetch context or preserve the reset/ranges used for that revision.
• P2 — An older revision summary incorporates later fetches. ?revision= limits staged rows, but the camera list and latest_attempts include all later fetches (app/services/flow_history_service.py, around lines 508–516). Re-reading an earlier revision after a later fetch can change its coverage and show later failures alongside older counts. The RFC says “earlier revisions stay readable” and calls this summary review evidence. Scope cameras and attempts to the requested revision's historical view.

Totals: Standards 2 findings (worst P2 client boundary); Spec 2 findings (both P2 coverage evidence). Under the ordinary second-pass gate, the P2 findings need fixes before merge; the P3 can be fixed here or tracked in a linked follow-up issue with its acceptance criterion.

## Review pass 2 — b07a0b9 (#162 retrieval and staging) Reviewed `a236666...b07a0b9`, including the pass-1 fixes in `fec62f3..b07a0b9`, against issue #162, the historical-flow RFC, AGENTS.md, and the code standards. The pass-1 fixes addressed the previously reported collision, camera, completeness, recovery, and test concerns. This pass found the following remaining issues. ## Standards - **P2 — Artemis response parsing crosses the client boundary.** `app/services/flow_history_service.py` (`_page_items`, `_accept_row`, around lines 425–487) reads the vendor envelope and fields `code`, `list`, `cameraIndexCode`, `enterNum`, and `exitNum`. `docs/standards/code-standards.md` §1.1 says clients must isolate third-party protocol details so services receive domain-oriented inputs and outputs. Move envelope and row decoding into `ArtemisClient` or a client-side adapter, returning typed page/evidence data. - **P3 — Possible Duplicated Code smell (judgement call).** `app/facility_time.py` and `app/schemas/flow_history.py` separately define the same `YYYY-MM` validation pattern. Sharing the pattern would keep month validation consistent across the domain and API schema. ## Spec - **P2 — Day evidence changes if the facility reset time changes.** `get_month_summary_async` builds day ranges with the *current* `daily_reset_time` (`app/services/flow_history_service.py`, around lines 507–532), while fetches retain their original request ranges. Changing the reset can move staged hours between days and report a recorded request as `cameras_never_requested`. The RFC requires “Every request's range ... [to be] stored with the fetch” and describes the month summary as per-business-day review evidence. Derive historical coverage from the persisted fetch context or preserve the reset/ranges used for that revision. - **P2 — An older revision summary incorporates later fetches.** `?revision=` limits staged rows, but the camera list and `latest_attempts` include all later fetches (`app/services/flow_history_service.py`, around lines 508–516). Re-reading an earlier revision after a later fetch can change its coverage and show later failures alongside older counts. The RFC says “earlier revisions stay readable” and calls this summary review evidence. Scope cameras and attempts to the requested revision's historical view. **Totals:** Standards 2 findings (worst P2 client boundary); Spec 2 findings (both P2 coverage evidence). Under the ordinary second-pass gate, the P2 findings need fixes before merge; the P3 can be fixed here or tracked in a linked follow-up issue with its acceptance criterion.

gabogg referenced this pull request 2026-09-28 10:34:40 +00:00

docs(agents): group feature deliveries into milestones #179

gabogg added 2 commits 2026-09-28 11:00:24 +00:00

Merge remote-tracking branch 'origin/master' into feat/historical-flow-backfill a0202afe45

fix(occupancy): address review pass 2 on historical flow staging (#162) ... 93df647257

- P2 (standards): decode the statisticsTotalNumByTime envelope and rows in
  the client layer. New app/clients/people_counting.py returns typed
  PeopleCountingPage/HourlyPeopleCount (numeric strings, naive facility
  times, malformed-row count, failure reasons); ArtemisClient takes
  datetimes and formats the request. The service keeps only domain checks
  (requested camera, whole hour, inside the range).
- P2 (spec): each fetch records the reset time its day ranges used and
  each request its business day; the month summary cuts days with the
  view's recorded reset and matches attempts by business day, so a later
  reset change no longer moves staged hours or hides requests. The summary
  reports the reset used and whether fetches in view disagree.
- P2 (spec): a revision's summary covers only fetches made before the next
  revision existed, for cameras, attempts and the fetch list.
- P3: MONTH_PATTERN is defined once in facility_time and reused by the
  API schemas.
- Tests: decoder typing and failure pages, reset change after a fetch,
  older revision unaffected by later fetches; the fake HikCentral now
  emits vendor JSON through the real decoder.

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

gabogg commented 2026-09-28 11:00:44 +00:00

Author

Owner

Copy link

Review pass 2: resolution in 93df647

All four findings are fixed on this PR. At the maintainer's request, the single P3 is fixed here instead of being filed as a follow-up issue.

Standards

• P2: Artemis response parsing crossed the client boundary.
  • New app/clients/people_counting.py decodes the statisticsTotalNumByTime envelope (code/msg/data) and its rows (time, cameraIndexCode, enterNum, exitNum) into typed PeopleCountingPage and HourlyPeopleCount values. It handles numeric strings, treats naive times as facility time, counts malformed rows and records why a page failed.
  • ArtemisClient.people_counting_by_hour_async now takes datetimes, formats the request itself and returns a typed page.
  • The service keeps only domain checks: requested camera, whole hour, inside the range.
• P3: duplicated YYYY-MM pattern. MONTH_PATTERN is now defined once in app/facility_time.py and reused by app/schemas/flow_history.py.

Spec

• P2: day evidence changed when the reset time changed.
  • Each fetch now records the reset_time its day ranges used, and each request records its business_day.
  • The summary cuts days with the view's recorded reset and matches attempts by business day, not by epoch.
  • It reports reset_time and flags reset_times_differ when fetches in the view used different resets.
• P2: an older revision's summary included later fetches. A revision's view now covers only fetches made before the next revision existed. That scope applies to the camera list, the latest attempts (filtered in SQL through before_fetch_id) and the returned fetch list.

Tests

• Decoder typing and failed pages.
• A reset change after a fetch leaves the summary unchanged.
• An older revision is unaffected by a later fetch that adds a camera and one that fails.
• The fake HikCentral now emits vendor JSON through the real decoder.

Verification

• Merged current master into the branch first (a0202af).
• Full suite: 450 passed. Ruff check and format clean; check_docs.py passes; pre-commit passed on commit.

Merging next, as the maintainer requested.

## Review pass 2: resolution in `93df647` All four findings are fixed on this PR. At the maintainer's request, the single P3 is fixed here instead of being filed as a follow-up issue. ### Standards - **P2: Artemis response parsing crossed the client boundary.** - New `app/clients/people_counting.py` decodes the `statisticsTotalNumByTime` envelope (`code`/`msg`/`data`) and its rows (`time`, `cameraIndexCode`, `enterNum`, `exitNum`) into typed `PeopleCountingPage` and `HourlyPeopleCount` values. It handles numeric strings, treats naive times as facility time, counts malformed rows and records why a page failed. - `ArtemisClient.people_counting_by_hour_async` now takes datetimes, formats the request itself and returns a typed page. - The service keeps only domain checks: requested camera, whole hour, inside the range. - **P3: duplicated `YYYY-MM` pattern.** `MONTH_PATTERN` is now defined once in `app/facility_time.py` and reused by `app/schemas/flow_history.py`. ### Spec - **P2: day evidence changed when the reset time changed.** - Each fetch now records the `reset_time` its day ranges used, and each request records its `business_day`. - The summary cuts days with the view's recorded reset and matches attempts by business day, not by epoch. - It reports `reset_time` and flags `reset_times_differ` when fetches in the view used different resets. - **P2: an older revision's summary included later fetches.** A revision's view now covers only fetches made before the next revision existed. That scope applies to the camera list, the latest attempts (filtered in SQL through `before_fetch_id`) and the returned fetch list. ### Tests - Decoder typing and failed pages. - A reset change after a fetch leaves the summary unchanged. - An older revision is unaffected by a later fetch that adds a camera and one that fails. - The fake HikCentral now emits vendor JSON through the real decoder. ### Verification - Merged current `master` into the branch first (`a0202af`). - Full suite: 450 passed. Ruff check and format clean; `check_docs.py` passes; pre-commit passed on commit. Merging next, as the maintainer requested.

gabogg merged commit f5732e85ca into master 2026-09-28 11:01:01 +00:00

gabogg deleted branch feat/historical-flow-backfill 2026-09-28 11:01:01 +00:00

gabogg referenced this pull request from a commit 2026-09-28 11:01:02 +00:00

feat(occupancy): retrieve and stage historical HikCentral flow (#166)

gabogg referenced this pull request 2026-09-28 11:08:16 +00:00

refactor(ui): fold diagnostic decks into one DIAGNOSTICS menu #173

gabogg referenced this pull request 2026-10-02 11:49:41 +00:00

docs(agents): post review passes as formal PR reviews #185

gabogg referenced this pull request 2026-10-02 11:50:11 +00:00

docs(agents): post review passes as formal PR reviews #185

gabogg referenced this pull request 2026-10-02 12:00:12 +00:00

docs(agents): post review passes as formal PR reviews #185

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

Historical passenger-flow backfill

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

No description provided.