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

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

Open
gabogg wants to merge 7 commits from feat/historical-flow-publication into feat/historical-flow-curation
pull from: feat/historical-flow-publication
merge into: gabogg:feat/historical-flow-curation
Conversation 4 Commits 7 Files changed 9 +1653 -5
gabogg commented 2026-09-28 09:01:01 +00:00
Owner
Copy link

Summary

Draft for #164: atomic approval and publication of a reviewed historical month. It covers source precedence (local observations own any observed camera-hour), long-lived local rollups that survive the 180-day event pruning, an independent quality result for hourly aggregates, and an explicit imported-or-mixed-source exclusion from calibration (both the immediate nightly multiplier update and EWMA/variance/sample history). It was split out of #166. Specification: #164 and the historical-flow RFC.

Stacked on feat/historical-flow-curation (#163 draft). Only a placeholder commit is on this branch; implementation has not started. It follows #163 in issue order.

Architectural impact (planned)

  • A separate, source-attributed approved hourly aggregate table (never people_counting_events).
  • Day-level coverage and provenance records.
  • Durable local daily/hourly rollups.
  • A guarded, single-transaction approval path.
  • A calibration guard in the nightly multiplier path and in history selection.

Verification

  • Tests for atomic rollback, repeated approval, upstream revision, local precedence, retention and source-aware quality; proof that imported/mixed days cannot change active k or enter calibration history.
  • Ruff, scripts/check_docs.py, full pytest.

Checklist

  • #163 merged or settled.
  • Implement #164.
  • Review passes.

Issues closed on merge

Closes #164

🤖 Generated with Claude Code

## Summary Draft for #164: atomic approval and publication of a reviewed historical month. It covers source precedence (local observations own any observed camera-hour), long-lived local rollups that survive the 180-day event pruning, an independent quality result for hourly aggregates, and an explicit imported-or-mixed-source exclusion from calibration (both the immediate nightly multiplier update and EWMA/variance/sample history). It was split out of #166. Specification: #164 and the historical-flow RFC. Stacked on `feat/historical-flow-curation` (#163 draft). Only a placeholder commit is on this branch; implementation has not started. It follows #163 in issue order. ## Architectural impact (planned) - A separate, source-attributed approved hourly aggregate table (never `people_counting_events`). - Day-level coverage and provenance records. - Durable local daily/hourly rollups. - A guarded, single-transaction approval path. - A calibration guard in the nightly multiplier path and in history selection. ## Verification - [ ] Tests for atomic rollback, repeated approval, upstream revision, local precedence, retention and source-aware quality; proof that imported/mixed days cannot change active k or enter calibration history. - [ ] Ruff, `scripts/check_docs.py`, full pytest. ## Checklist - [ ] #163 merged or settled. - [ ] Implement #164. - [ ] Review passes. ## Issues closed on merge Closes #164 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Placeholder commit so the stacked draft PR exists before implementation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-10-02 13:35:57 +00:00
Author
Owner
Copy link

Marked blocked: implementation is blocked by PR #178 (feat(statistics): holiday and event context for investor analytics), which is the remaining prerequisite needed to build the parent curation drafts in PR #170 (feat(occupancy): curate monthly historical flow drafts / #163). PR #171 is stacked on PR #170 and provides the atomic approval/publication transaction for those reviewed drafts.

Marked blocked: implementation is blocked by PR #178 (`feat(statistics): holiday and event context for investor analytics`), which is the remaining prerequisite needed to build the parent curation drafts in PR #170 (`feat(occupancy): curate monthly historical flow drafts` / #163). PR #171 is stacked on PR #170 and provides the atomic approval/publication transaction for those reviewed drafts.
gabogg commented 2026-10-03 09:07:37 +00:00
Author
Owner
Copy link

Unblocked: #178 merged (b64e805). Master is now merged up the stack: #170 and #171 are at the new master. #170 stays blocked, since its prerequisite #177 is still in review.

Unblocked: #178 merged (`b64e805`). Master is now merged up the stack: #170 and #171 are at the new master. #170 stays `blocked`, since its prerequisite #177 is still in review.
gabogg commented 2026-10-03 11:56:17 +00:00
Author
Owner
Copy link

Merged #170 (feat/historical-flow-curation @ 888572c) into feat/historical-flow-publication. No conflicts encountered. Full test suite (612 tests) passes cleanly.

Merged #170 (feat/historical-flow-curation @ 888572c) into feat/historical-flow-publication. No conflicts encountered. Full test suite (612 tests) passes cleanly.
gabogg changed title from WIP: feat(occupancy): publish reviewed historical flow without calibrating to feat(occupancy): publish reviewed historical flow without calibrating 2026-10-03 12:54:29 +00:00
gabogg commented 2026-10-03 12:54:47 +00:00
Author
Owner
Copy link

Implementation complete against #164 acceptance criteria and maintainer decisions of 2026-10-02 on #170.

  • Atomic single-transaction approval/publication of reviewed monthly historical flow draft.
  • Source precedence: local observations authoritative for wholly observed camera-hours; imported HikCentral counts used only where local is wholly absent; partly observed hours left unresolved.
  • Approved hourly aggregates stored separately in flow_history_approved_*, never entering people_counting_events.
  • Long-lived local daily/hourly rollups surviving 180-day raw event pruning.
  • Independent quality result for hourly aggregates; no automatic nocturnal audit fabricated from imported aggregates.
  • Strict calibration exclusion: imported or mixed-source days cannot update active exit multiplier k or enter calibration history.
  • Original Schedule fixed explicitly on approval/publication (ADR 0008).
  • Master invariants preserved: #178 holiday identity single source, #177 pending changes and effective reset boundaries, #234 BEGIN IMMEDIATE schema wrapper.

All 621 pytest tests, 248 node tests, ruff format/check, and check_docs.py pass cleanly. Ready for pass 1 review.

Implementation complete against #164 acceptance criteria and maintainer decisions of 2026-10-02 on #170. - Atomic single-transaction approval/publication of reviewed monthly historical flow draft. - Source precedence: local observations authoritative for wholly observed camera-hours; imported HikCentral counts used only where local is wholly absent; partly observed hours left unresolved. - Approved hourly aggregates stored separately in `flow_history_approved_*`, never entering `people_counting_events`. - Long-lived local daily/hourly rollups surviving 180-day raw event pruning. - Independent quality result for hourly aggregates; no automatic nocturnal audit fabricated from imported aggregates. - Strict calibration exclusion: imported or mixed-source days cannot update active exit multiplier k or enter calibration history. - Original Schedule fixed explicitly on approval/publication (ADR 0008). - Master invariants preserved: #178 holiday identity single source, #177 pending changes and effective reset boundaries, #234 BEGIN IMMEDIATE schema wrapper. All 621 pytest tests, 248 node tests, ruff format/check, and check_docs.py pass cleanly. Ready for pass 1 review.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg reviewed 2026-10-03 18:14:12 +00:00
gabogg left a comment
Copy link

Code review, pass 1 (origin/pr/170...5742d63, spec #164 + #163 decisions)

Summary: 5 distinct P1s (after removing duplicates), about 12 P2s, and P3s. Pass 1: fix everything. Judged on #171's own diff over #170 (8c78911).

P1s:

  1. The reconcile loop never ends once an approved day lacks a calibration log (Standards P1-1, occupancy_service.py:2496-2499): the continue skips the cursor advance. It is reachable from the analytics POST.
  2. A timed event annotation breaks every trusted-history reader (Standards P1-2). Raw SQL inserts the event with NULL epochs, and get_trusted_calibration_history_async then raises a TypeError. That takes down nightly calibration, live state and analytics. Use OccupancyManager.create_event_async, or a shared conn-level helper.
  3. Approving a month marks lived local days IMPORTED (Standards P1-3, Spec P2-4). Those days drop out of calibration history, and dated exceptions (is_exception, exception_name) are wiped from the business-day record, which overrides the ADR 0007 correction policy. Only purely imported days become IMPORTED. Mixed or lived days keep their record, and #178's record stays the source of holiday and exception identity.
  4. Re-approval destroys the previous published revision (Spec P1-2, Standards P2-6). #164 requires keeping earlier snapshots, so version the approved tables by revision.
  5. The CHECK widening never reaches an existing database (Spec P1-1, Standards P2-4). Resolution: #170 owns the APPROVED and APPROVE CHECK values before it merges (this was added to #170's pass-2 fix list). #171 then needs no rebuild migration; drop the widening from #171 once #170 has it.

Key P2s: the approved-revision diff is missing (r53 moved it to #171, so it belongs here); approval must require PREPARED_FOR_APPROVAL and re-check status and revision inside BEGIN IMMEDIATE (add expected_revision and return 409); APPROVED drafts must be immutable; local precedence must survive pruning, so readers use the rollups; rollups exclude reconstructed slices and cameras that aren't counted; an empty COMPLETE day is not trusted; quality thresholds read the config dict (cfg.get), with no cap at 6; partly observed hours stay unresolved; the repository holds no domain logic and makes no service callbacks; the publication test uses isolated_repository_db; re-approval replaces stale annotations.

Coordination with #228: #228 adds occupancy_business_day_schedules.original_value. Whichever PR lands second writes original_value at approval, instead of using a STAMP audit row (the option ADR 0008 rejected).

Order: the #170 pass-2 fixes land first. Merge #170 into #171 before fixing here.

Standards axis

Scope: git diff origin/pr/170...origin/pr/171 (db9d606 plus placeholder eee9b1e). Line numbers are at 5742d63.
Probes ran in a git-archive export under scratchpad/r171/src, using a throwaway test with isolated_repository_db; it has been deleted. Ruff check and format are clean. tests/test_flow_history_publication.py passes (9 tests in 65 s).

P1

P1-1. The reconcile loop never ends once a day has imported flow. CONFIRMED (probe)
app/services/occupancy_service.py:2496-2499. The new continue skips the loop's only advance, cursor_cycle_start = cursor_cycle_end + CYCLE_END_EPSILON (about line 2580).
Failure: a local event exists before an approved month, and one approved day has no calibration log. reconcile_historical_calibrations_async then spins forever on the same cycle and hits the DB on every iteration. This path is reached from POST in analytics_controller.py:326 and from reconcile_and_quarantine_historical_anomalies_async (line 2616).
Probe: local event on 2026-07-30, then 2026-08 approved. asyncio.wait_for(..., 15) timed out.
Fix: advance the cursor before continue, or restructure the loop. Add a regression test.

P1-2. Approving a day with a timed event annotation breaks every trusted-calibration-history reader. CONFIRMED (probe)
app/db/flow_history_repository.py:1386-1430. The approval inserts the event with raw SQL: is_all_day=0 when both times are set, start_epoch/end_epoch NULL, and start_time="18:00" (the service stores ISO datetimes). occupancy_repository.py:2970-2975 (and 3443) then runs active_business_day(r["start_epoch"], ...) and r["end_epoch"] - 0.001.
Failure: a curation day with event_annotation="Concert", event_start_time="18:00", event_end_time="22:00" is approved. After that, get_trusted_calibration_history_async raises TypeError: unsupported operand type(s) for -: 'NoneType' and 'float'. The affected callers are nightly calibration (occupancy_service.py:2012, 2112), live state (934, 2956) and analytics (analytics_service.py:1699).
Root cause: the repository bypasses OccupancyManager.create_event_async, which computes the epochs and validates them. The same code shape is behind the layering finding P2-1.

P1-3. Approving a month marks every day IMPORTED, including locally lived days, which then leave calibration history. CONFIRMED (probe)
app/db/flow_history_repository.py:1298-1331 upserts occupancy_business_day_schedules.source='IMPORTED' for every day in the month. occupancy_repository.py:3046-3048 / 3476-3478 and has_imported_or_mixed_flow_async (3279) exclude any day whose record is IMPORTED, whatever the day's flow source.
Failure: 2026-08 was lived locally, and an admin approves it to fill a few gaps. All 31 days get source=IMPORTED. Their existing trusted calibration logs disappear from EWMA/variance/sample-history selection, so the next nightly k changes. The probe showed a day with only local hours (approved source='local_events'): its schedule became IMPORTED and has_imported_or_mixed_flow_async returned True. The upsert also overwrites the current record (WEEKLY/EXCEPTION/MANUAL) of lived days with draft values, outside the correction and next-day-activation policy (ADR 0007).
This contradicts #164 ("do not rewrite already-applied calibration"; the exclusion covers only imported or mixed days) and ADR 0008 (only imported days are "marked as imported"; a lived day's record is its own).

P2

P2-1. Layering: the repository holds domain logic, calls back into the service, and writes another aggregate's tables. CONFIRMED

  • flow_history_repository.py:1103-1118: get_curation_draft_async and approve_curation_draft_async import flow_history_service from inside the repository, a circular, inverted dependency. Only tests use them (test_flow_history_publication.py:310, 354). code-standards §1.1 says repositories only persist.
  • store_approved_month_async (1120-1479) holds domain rules: day status tallies, day-source classification, the open-window computation and the whole quality verdict (1200-1221). It also hand-writes occupancy schedules, schedule audit, events and event audit, duplicating the OccupancyRepository/OccupancyManager writers (Shotgun Surgery, Duplicated Code). That duplication is how P1-2 happened.
  • Fix: the service computes ApprovedDayRecords and the quality verdict. The repository receives typed records and persists them in one transaction, using shared connection-level helpers from the occupancy repository for schedule and event writes.

P2-2. Approval atomicity: every check runs outside the write transaction, and the final UPDATE has no guard. PLAUSIBLE (race; the code path is confirmed by reading)
flow_history_service.py:1246-1310 reads the draft and all 31 day details, then calls store_approved_month_async, which only starts BEGIN IMMEDIATE at repo:1136. Inside the transaction nothing re-checks the draft's status or revision, and UPDATE ... SET status='APPROVED' WHERE month=? (1433-1440) has no status predicate and no rowcount check.
Failure: admin A approves while admin B discards, or patches a day back to UNREVIEWED, in between. The publication is built from the stale read, and the DISCARDED draft is resurrected as APPROVED with its curation_days already deleted.
Also, CurationApproveRequest has no expected_revision field, although every other #170 mutation takes one.
Fix: re-read the draft row under BEGIN IMMEDIATE, require the expected status and revision, and raise DraftConflictError (409) on mismatch.

P2-3. Approval skips the PREPARED_FOR_APPROVAL lifecycle, and APPROVED drafts stay editable. CONFIRMED

  • flow_history_service.py:1247 rejects only DISCARDED, so a plain DRAFT can be approved directly (the probe printed "status before approve: DRAFT"; no test calls prepare-approval). The approve gates (1259-1280) duplicate prepare's gates (1149-1219) and have drifted from them. For example, approval does not re-check camera_mismatch_reason text, and it has no reason gate tied to unresolved_camera_hours or suggested_status.
  • #171 adds APPROVED, but the #170 mutation guards (service 831/907/960/1087; repo 605-607/655-657/721-723/804-806) block only PREPARED_FOR_APPROVAL. An APPROVED draft therefore accepts PATCH day/schedule/zero-confirm edits and drifts from what was published.
  • Fix: require status == 'PREPARED_FOR_APPROVAL' (checked in the transaction, see P2-2), rely on prepare's gates, and treat APPROVED as immutable, like PREPARED.

P2-4. Migration: the status/action CHECK changes in CREATE TABLE IF NOT EXISTS never reach an existing database. CONFIRMED (probe)
app/db/database.py:866, 933. Probe: initialise the DB with the pr/170 tree, then with pr/171. UPDATE ... status='APPROVED' fails with CHECK constraint failed: status IN ('DRAFT','PREPARED_FOR_APPROVAL','DISCARDED'), and the 'APPROVE' audit insert fails the same way.
Failure: if #170 merges and deploys before #171 (they are separate PRs), every approval on that DB returns 500 permanently.
Fix: move the APPROVED/APPROVE values into #170 before it merges, or add a table-rebuild migration (#228 introduces a generic CHECK-rebuild helper).

P2-5. Quality thresholds ignore admin configuration; an empty COMPLETE day is trusted. CONFIRMED (probe for the second part)

  • flow_history_service.py:1282-1286: getattr(cfg, "trust_ratio_min", 0.80) etc. run on the dict returned by get_config_async, so they always return the hard-coded defaults. occupancy_service.py:2085-2165 correctly uses cfg.get(...). The same getattr(cfg, "holiday_open_time") bug exists in #170 (service ~993); report it to #170.
  • flow_history_repository.py:1206: when total_vol == 0, every check is skipped and is_trusted=1. Probe: a day with no data, all hours NONE, marked COMPLETE with a reason, was published is_trusted=True with 0 counts. #164 says "Admin coverage approval does not itself assert sensor trust".
  • :1220: min(6, trust_min_active_hours) silently caps the configured minimum at 6 (a magic number).

P2-6. Re-approval destroys the prior published snapshot. CONFIRMED (reading)
flow_history_repository.py:1139-1144 deletes the previous flow_history_approved_days/_hours, and the months row is upserted in place. #164 requires that re-approval "replaces the published revision atomically while retaining prior snapshots". The test at test_flow_history_publication.py:531-543 checks only the staging rows. This overlaps the spec axis.

P2-7. Partly observed hours lose their unresolved state in publication, and open-window counts disagree with day totals. CONFIRMED (reading)

  • database.py:975-987 gives flow_history_approved_hours no unresolved flag. The repository (1262-1268) writes partly observed LOCAL hours as source='local_events' with their partial counts, which cannot be told apart from wholly observed hours. #164: "Leave partially observed hours unresolved". The cam02 case in the precedence test is seeded but never asserted.
  • :1191-1198: open_window_enter sums enter_num over all hours, including unresolved partly observed ones, while enter_total excludes them. So open_window_enter can exceed enter_total. The open-window computation also truncates minutes (open/close "09:30"/"21:30" count by hour only), returns 0 when the close time is past midnight, and parses the hour by slicing hour_start[11:13] (Primitive Obsession).

P2-8. Local rollups count data that published KPIs exclude. CONFIRMED (reading)
occupancy_repository.py:3206-3226 sums every people_counting_events row, including reconstructed gap slices (raw_payload.reconstructed) and excluded or inactive cameras. The KPI and trust queries filter both out (_COUNTED_CAMERA_JOIN/FILTER, 3355-3358).
Failure: year-over-year comparisons built on rollups older than 180 days are inflated relative to live statistics for the same period.
Related, P3-level, PLAUSIBLE: the business day is attributed with the current daily_reset_time instead of the effective-dated reset (ADR 0007).

P2-9. The new test module bypasses isolated_repository_db. CONFIRMED (reading)
tests/test_flow_history_publication.py:43-67 uses an autouse fixture that DELETEs 20 tables in the session-wide DB, including occupancy_calibration_logs, occupancy_events and occupancy_business_day_schedules, which other modules rely on. Its sibling test_flow_history_curation.py:40 uses pytestmark = usefixtures("isolated_repository_db"). #233 and 1885b27 on master were exactly this class of order dependence.

P3

  1. Validation: CurationApproveRequest (schemas/flow_history.py:427-428) lacks the strip validator that every sibling request has. The probe stored reason ' x '.
  2. Dead code / Speculative Generality:
    • draft.status == "DISCARDED" (service:1247) can never be true, because get_curation_draft_row_async returns None for DISCARDED, which yields a 404.
    • FlowHistoryRepository.has_imported_or_mixed_flow_async (1591-1616) duplicates the occupancy one and is unused. The occupancy copy (3279-3304) hard-codes 'hikcentral_artemis_hourly' instead of HISTORICAL_FLOW_SOURCE.
    • FlowHistoryService.get_approved_day_async (1318) has no route.
    • The reason or "Historical month approval" fallbacks (1361, 1377, 1426) can never fire.
  3. Typing:
    • day_details: list[Any] (repo:1124) should be list[CurationDayDetail].
    • get_curation_draft_async (repo:1103) has no return annotation.
  4. Data Clump: the five trust_* parameters plus now travel together into a 12-argument store_approved_month_async. Bundle the thresholds into one type, or read them inside the service-side quality function.
  5. Mysterious Name: events_rolled_up (occupancy_repository.py:3317) counts camera-hour groups, not events (the test expects 1 for 2 events).
  6. Event re-approval: dedupe by (start_date, name) (repo:1386-1391) means changed event times on re-approval are silently ignored.
  7. N+1: approval calls get_curation_day_detail_async once per day (service:1289-1296), and each call re-queries the whole month. #170 batched these queries for the month view.
  8. Test gaps:
    • GET /months/{month}/approved has no test, although code-standards §4 requires one for every endpoint.
    • Unasserted responses in the setup steps: test lines 225-228, 261-268, 498-500, 523-525, 662-664, 708-710.
    • fetch_id is unused at line 197.
    • The rollback test asserts only the month's reason; it should also check that the day and hour rows are intact. It also goes through the test-only repository seam rather than the route.
    • Missing regressions: the P1-1 loop, timed events (P1-2), local days staying calibration-eligible (P1-3), concurrency or stale revision (P2-2), approve-from-DRAFT refused (P2-3), and partly observed hours (P2-7).
  9. Docs:
    • CONTEXT.md does not define the new domain terms: approved historical flow, local rollup, hourly-aggregate quality result, day source mixed.
    • The schema comment at database.py:944-946 says the tables are "Consumed by read-only statistics deck and baselines", but nothing consumes them yet.
  10. PR hygiene (git-and-workflow): the PR body is stale. It still says "Only a placeholder commit ... implementation has not started", and the Verification and Checklist boxes are unchecked although the 08:54 comment reports them done. The "Architectural impact (planned)" section should describe what was built.

Notes: master and stack

  • No textual conflict with master: the merge base is 9a608c1.
  • Semantic clash with open PR #228 (same milestone). It adds an explicit original_value column and states that imported days "remain unfixed until approval". #171 fixes the Original Schedule only through a STAMP audit row (repo:1350-1365), which is the option ADR 0008 rejected. The test at line 305-307 depends on the audit-row reader. Whichever PR lands second must write original_value at approval and drop the audit-row dependency.
  • #170 carries the same getattr(cfg, ...)-on-dict bug for the holiday hours (see P2-5) and should own the CHECK values (see P2-4).

Spec axis

Spec: #164, the umbrella and decisions in #163, CONTEXT.md, ADR 0006, 0007 and 0008.

Unless stated otherwise, line numbers refer to app/db/flow_history_repository.py at 5742d63. Probes ran in a git-archive export with the suite's temp DB. The probe file was deleted afterwards.

Verification: these files pass:

  • tests/test_flow_history_publication.py (9 tests)
  • tests/test_flow_history_curation.py (17 tests)
  • tests/test_calibration_{audit_trust,honesty,reconciliation}.py

ruff check and ruff format --check are clean.

r53 ownership check

  • Tables: #170 (8c78911) has no flow_history_approved_* table. #171 now creates and writes all three (app/db/database.py:946-995). Done.
  • Approved-revision diff: this was dropped, not moved. Nothing in #171 compares a draft with the previously approved revision (see Spec P2-1).

P1

P1-1. Approval always fails on a database that already ran #170's schema. CONFIRMED.

  • Where: app/db/database.py:866 and :933.

  • Cause: #171 widens two CHECK constraints inside CREATE TABLE IF NOT EXISTS:

    • flow_history_curation_drafts.status gains APPROVED;
    • flow_history_curation_audit.action gains APPROVE.

    IF NOT EXISTS never changes an existing table, and there is no migration.

  • Probe: I ran init_schema from the #170 tree, then from the #171 tree. Afterwards, UPDATE ... SET status='APPROVED' failed with CHECK constraint failed: status IN ('DRAFT','PREPARED_FOR_APPROVAL','DISCARDED'), and the APPROVE audit insert failed the same way.

  • Failure scenario: #170 merges and deploys, then #171 deploys. Every POST /approve rolls back with a 500, so no month can ever be published.

  • Fix: either put both CHECK values in #170, or add a table-rebuild migration in #171. Test it with an upgrade from an existing database.

P1-2. Re-approval destroys the previously published revision. CONFIRMED.

  • Where: lines 1139-1144, plus create_curation_draft_record_async at ~495-503.
  • Spec: "Re-approval … replaces the published revision atomically while retaining prior snapshots and actor/time/reason audit records."
  • Cause: the approval deletes every flow_history_approved_days and flow_history_approved_hours row for the month and upserts the single flow_history_approved_months row. Creating the revision-2 draft has already deleted the revision-1 flow_history_curation_days, which hold the day reviews and reasons.
  • Probe: I approved revision 1 with day reason "R1 reason", then fetched revision 2, drafted, reviewed and approved it. The result was 0 rows with "R1 reason", only the revision-2 hours, and 1 month row. Revision 1's day statuses, reasons, provenance, local-versus-HikCentral resolution and hours are gone. What remains:
    • the staged HikCentral rows;
    • one APPROVE audit row holding month totals.
  • Why the test misses it: test_repeated_approval_replaces_published_revision checks only staging rows and the count of APPROVE audit rows.
  • Fix: keep published revisions, for example keyed by (month, published_revision) with a current pointer, or move superseded rows into history tables. The prior snapshot must stay queryable.

P2

P2-1. The "differences from a previously approved revision" view is gone. CONFIRMED (code read).

  • Spec: #163 acceptance asks the review to show differences from a previously approved revision.
  • What happened: r53 (P2-10) asked for the diff to move into #171 next to its writer. Neither #170 nor #171 has it now. No field in the schemas compares a draft with the approved tables (git grep for approved, previous or diff in app/schemas/flow_history.py and app/services/flow_history_service.py).
  • Failure scenario: an admin re-reviews August after a HikCentral revision and cannot see which days or counts changed from what is published.
  • Fix: add the diff in #171, against the retained prior snapshot from P1-2. It should compare status, enter and exit totals, and source. Add a test.

P2-2. Approval skips the prepare step and does not lock the published draft. CONFIRMED.

  • Where: app/services/flow_history_service.py:1247 checks only DISCARDED. The update guards at 831, 907, 960 and 1087 reject only PREPARED_FOR_APPROVAL.
  • Probes:
    • (a) A DRAFT that was never prepared approved with a 200, so #170's prepare gates, its reason and its PREPARE_APPROVAL audit are optional.
    • (b) Approving the same draft twice gave 200 and 200. It republished the same revision, wrote a second APPROVE audit, and doubled the schedule audit rows from 31 to 62 (31 extra OVERWRITE rows).
    • (c) After approval, PATCH /draft/days/{d} returned 200 on the APPROVED draft. The draft and the published month now disagree, and another approve would publish the edit without preparation.
  • Spec: #164 says "Require … validated day reviews". It also asks to "Test … repeated approval".
  • Fix:
    • Approve only PREPARED_FOR_APPROVAL drafts.
    • Make an approval of an already-approved draft an idempotent no-op or a 409.
    • Treat APPROVED as immutable on every edit path.

P2-3. Approval reads the draft outside its transaction and never re-checks it. CONFIRMED.

  • Where: the draft and day details are built at app/services/flow_history_service.py:1245-1296, before BEGIN IMMEDIATE (line 1136). Step 3 (line 1432) updates the draft with no status or revision predicate.
  • Probe: I made store_approved_month_async discard the draft just before it ran. Approval still published all 31 days and flipped the DISCARDED draft to APPROVED. Its curation days had already been deleted.
  • The same window allows two more failures:
    • an edit landing between the read and the write gets published stale;
    • two concurrent approvals both succeed.
  • Fix: inside the transaction, re-read the draft row. Require status = 'PREPARED_FOR_APPROVAL' and the expected revision, or use UPDATE … WHERE status=? AND revision=? and check rowcount.

P2-4. Approval rewrites the business-day record of locally lived days as IMPORTED. CONFIRMED.

  • Where: lines 1304-1331. The approval upserts source='IMPORTED' for every day of the month and recomputes is_exception and exception_name from the draft's day_type.
  • Probe: I seeded a lived day with an EXCEPTION record ("Aniversario", is_exception=1), a FREEZE audit row, and local events in every camera-hour. After approval:
    • the approved day source was local_events;
    • the business-day record source was IMPORTED, with is_exception=0 and exception_name=None;
    • has_imported_or_mixed_flow_async returned True.
  • Consequences:
    1. A purely local day leaves calibration history. Both history queries filter source = 'IMPORTED' (app/db/occupancy_repository.py:3047 and :3477), and the nightly path filters through has_imported_or_mixed_flow_async. The spec excludes only imported-or-mixed days. Publication should not change the calibration eligibility of a lived day.
    2. The dated exception's identity is erased from the record that #178 makes authoritative. ADR 0008 marks only imported days as imported.
  • Fix: write or overwrite the record only for days without a lived record, or only for days whose source is imported or mixed. Never re-source a lived day. Keep is_exception and exception_name from the existing record.

P2-5. Local precedence is lost once raw events are pruned, and the local rollups are never read. CONFIRMED.

  • Where: get_month_local_events_async (~985) still reads only people_counting_events. Nothing in app/ reads flow_history_local_*_rollups.
  • Probe: month 2026-03, which is more than 180 days old. cam01 had a local event of 99 at 12:00, and HikCentral had staged 7 for the same hour. Before pruning, the curation hour was LOCAL 99. After prune_retention_data_async, it became HIKCENTRAL 7.
  • Failure scenario: an older month is approved after pruning. Imported counts then overwrite camera-hours that were observed locally, against the "local observations own any observed camera-hour" invariant.
  • Note: #164 also says the rollups exist "so year-over-year comparisons survive", but no consumer or read path ships. The #160 statistics child can consume them, but curation precedence belongs here.
  • Fix: have curation's local evidence take the union of raw events and hourly rollups. The rollup needs to mark the camera-hour as observed, including zero counts.

P2-6. The rollups count reconstructed estimates and cameras that are not counted. CONFIRMED.

  • Where: app/db/occupancy_repository.py:3206-3221.
  • Cause: the rollup query sums every people_counting_events row. It has no _COUNTED_CAMERA_FILTER or join, and no reconstructed exclusion. The published KPIs and get_hourly_flow_distribution_async apply both.
  • Probe: a real event of 99, a reconstructed slice of 500, and 300 from an unregistered camXX produced a daily rollup of 899. Every row was labelled source='local_events'.
  • Failure scenario: a year-over-year comparison reads a day at about nine times its counted traffic, and gap estimates pass for observations.
  • Fix: apply the same counted-camera and non-reconstructed filters. Store reconstructed volume separately if it is needed at all.

P2-7. The quality result trusts a Complete day that has no counts. CONFIRMED.

  • Where: lines 1200-1221.

  • Cause: when total_vol == 0, every check is skipped and is_trusted stays 1.

  • Probe: with nothing staged, every day had 288 unresolved hours. All days were set to COMPLETE with a reason, then approved. The result: status=COMPLETE, enter_total=0, is_trusted=True, the day source was the HikCentral source, and all 288 hours had source='NONE'.

  • Spec:

    • "Admin coverage approval does not itself assert sensor trust."
    • "A Complete imported day can be eligible … after quality checks."

    Here the admin's coverage assertion alone produces a trusted, comparison-eligible empty day.

  • Fix: zero volume, and any NONE hour on a Complete day, should give an untrusted result with a flag.

P2-8. The independent quality result is weaker than the live R1-R4 checks and ignores the admin's configuration. CONFIRMED (code read).

  • Configuration ignored: app/services/flow_history_service.py:1282-1286 uses getattr(cfg, …) on the dict returned by get_config_async, so the configured trust thresholds are always replaced by the hard-coded defaults.
  • Threshold clamped: line 1220 uses min(6, trust_min_active_hours), which silently lowers R1 from 10 to 6.
  • Checks missing: there is no minimum-footfall check (R4) and no per-hour gap awareness.
  • No flags stored: only a boolean is stored, so no one can see why a day is untrusted.
  • Fix: use cfg.get(...) and the live thresholds. Store the flags with the result.

P2-9. A re-approval leaves stale event annotations. PLAUSIBLE (code read, not probed).

  • Where: lines 1385-1430.
  • Cause: events are inserted only if a matching (start_date, name) is missing, and nothing removes them.
  • Failure scenario: revision 1 annotates "Concierto" on 08-14. Revision 2 removes or renames it. The old event stays in occupancy_events, so the day remains an Event Day for statistics and is excluded from learning. A changed annotation creates a second event.
  • Spec: "replaces the published revision atomically."
  • Fix: track the events created by an approval, by approval revision. Replace or retire them on re-approval, with an audit entry.

P3

  1. A Missing day is labelled as HikCentral data (line 1186). A day whose hours are all NONE gets the HikCentral source. That is misleading provenance, and it also triggers the calibration exclusion. A neutral value such as none would be clearer.
  2. The rollups use today's reset time (app/db/occupancy_repository.py:3244). active_business_day(h_epoch, reset_time) takes the current daily_reset_time, not the ADR 0007 effective-dated reset (reset_for(day)). After a reset-time change, old hours are assigned to the wrong business day. Hourly buckets also straddle a reset that is not on the hour.
  3. Open-window counts truncate opening hours (lines 1189-1198). open_window_enter uses whole hours only, so 10:30 becomes 10:00. It returns 0 for a close time after midnight.
  4. The nocturnal-audit skip also drops local data (app/services/occupancy_service.py:2498). It skips mixed days and lived days re-sourced by P2-4. The spec forbids an audit "solely from imported aggregates", so a mixed day's local data could still be audited. Mostly fixed by P2-4.
  5. Legacy calibration logs can vanish from history (app/db/occupancy_repository.py:3049-3052). The new cycle_date NOT IN (subquery) clause drops logs whose cycle_date is NULL once any approved day exists, because NULL NOT IN a non-empty set is not true. PLAUSIBLE, since the holiday clause already behaves this way.
  6. The repository calls the service (lines 1103-1118). get_curation_draft_async and approve_curation_draft_async in the repository import and call the service, which inverts the layering. The rollback test depends on these shims.
  7. The approved-day read has no route. get_approved_day_async exists in the service, but no route exposes it. Hour-level approved data is unreachable over the API.
  8. The Original Schedule still comes from the oldest audit row. Approval writes it through a STAMP audit row (the comment cites ADR 0008), but ADR 0008, which is accepted, rejected exactly that inference. This is fine for now, because master has no stored column yet. Note it in the PR as owed to #186 item 7.
  9. The PR body is stale. It says "Only a placeholder commit … implementation has not started", lists the architecture as planned, and has unchecked verification. It also does not mention the new approve and approved routes, or that the approval writes business-day records and events.
  10. Missing tests:
    • approval from a DRAFT that was never prepared;
    • double approval;
    • editing an approved draft;
    • concurrent discard or edit during approval;
    • upgrade from an existing #170 schema;
    • local precedence after pruning;
    • rollup filters;
    • a zero-volume Complete day;
    • a lived day inside an approved month;
    • re-approval with a changed event annotation;
    • retention of the prior snapshot.
  11. CONTEXT.md has no glossary entries for the approved hourly aggregate, the local rollup, or a mixed-source day.

Master

  • The merge base is master 9a608c1, and master has not changed any of these files since. I expect no textual conflicts.
  • Semantic risk: P1-1, if #170 deploys on its own.
## Code review, pass 1 (`origin/pr/170...5742d63`, spec #164 + #163 decisions) **Summary: 5 distinct P1s (after removing duplicates), about 12 P2s, and P3s. Pass 1: fix everything.** Judged on #171's own diff over #170 (8c78911). P1s: 1. **The reconcile loop never ends** once an approved day lacks a calibration log (Standards P1-1, `occupancy_service.py:2496-2499`): the `continue` skips the cursor advance. It is reachable from the analytics POST. 2. **A timed event annotation breaks every trusted-history reader** (Standards P1-2). Raw SQL inserts the event with NULL epochs, and `get_trusted_calibration_history_async` then raises a TypeError. That takes down nightly calibration, live state and analytics. Use `OccupancyManager.create_event_async`, or a shared conn-level helper. 3. **Approving a month marks lived local days IMPORTED** (Standards P1-3, Spec P2-4). Those days drop out of calibration history, and dated exceptions (`is_exception`, `exception_name`) are wiped from the business-day record, which overrides the ADR 0007 correction policy. Only purely imported days become IMPORTED. Mixed or lived days keep their record, and #178's record stays the source of holiday and exception identity. 4. **Re-approval destroys the previous published revision** (Spec P1-2, Standards P2-6). #164 requires keeping earlier snapshots, so version the approved tables by revision. 5. **The CHECK widening never reaches an existing database** (Spec P1-1, Standards P2-4). Resolution: #170 owns the `APPROVED` and `APPROVE` CHECK values before it merges (this was added to #170's pass-2 fix list). #171 then needs no rebuild migration; drop the widening from #171 once #170 has it. Key P2s: the approved-revision diff is missing (r53 moved it to #171, so it belongs here); approval must require PREPARED_FOR_APPROVAL and re-check status and revision inside `BEGIN IMMEDIATE` (add `expected_revision` and return 409); APPROVED drafts must be immutable; local precedence must survive pruning, so readers use the rollups; rollups exclude reconstructed slices and cameras that aren't counted; an empty COMPLETE day is not trusted; quality thresholds read the config dict (`cfg.get`), with no cap at 6; partly observed hours stay unresolved; the repository holds no domain logic and makes no service callbacks; the publication test uses `isolated_repository_db`; re-approval replaces stale annotations. **Coordination with #228:** #228 adds `occupancy_business_day_schedules.original_value`. Whichever PR lands second writes `original_value` at approval, instead of using a STAMP audit row (the option ADR 0008 rejected). **Order:** the #170 pass-2 fixes land first. Merge #170 into #171 before fixing here. ### Standards axis Scope: `git diff origin/pr/170...origin/pr/171` (db9d606 plus placeholder eee9b1e). Line numbers are at 5742d63. Probes ran in a git-archive export under `scratchpad/r171/src`, using a throwaway test with `isolated_repository_db`; it has been deleted. Ruff check and format are clean. `tests/test_flow_history_publication.py` passes (9 tests in 65 s). #### P1 **P1-1. The reconcile loop never ends once a day has imported flow.** CONFIRMED (probe) `app/services/occupancy_service.py:2496-2499`. The new `continue` skips the loop's only advance, `cursor_cycle_start = cursor_cycle_end + CYCLE_END_EPSILON` (about line 2580). Failure: a local event exists before an approved month, and one approved day has no calibration log. `reconcile_historical_calibrations_async` then spins forever on the same cycle and hits the DB on every iteration. This path is reached from `POST` in analytics_controller.py:326 and from `reconcile_and_quarantine_historical_anomalies_async` (line 2616). Probe: local event on 2026-07-30, then 2026-08 approved. `asyncio.wait_for(..., 15)` timed out. Fix: advance the cursor before `continue`, or restructure the loop. Add a regression test. **P1-2. Approving a day with a timed event annotation breaks every trusted-calibration-history reader.** CONFIRMED (probe) `app/db/flow_history_repository.py:1386-1430`. The approval inserts the event with raw SQL: `is_all_day=0` when both times are set, `start_epoch`/`end_epoch` NULL, and `start_time="18:00"` (the service stores ISO datetimes). `occupancy_repository.py:2970-2975` (and 3443) then runs `active_business_day(r["start_epoch"], ...)` and `r["end_epoch"] - 0.001`. Failure: a curation day with `event_annotation="Concert", event_start_time="18:00", event_end_time="22:00"` is approved. After that, `get_trusted_calibration_history_async` raises `TypeError: unsupported operand type(s) for -: 'NoneType' and 'float'`. The affected callers are nightly calibration (occupancy_service.py:2012, 2112), live state (934, 2956) and analytics (analytics_service.py:1699). Root cause: the repository bypasses `OccupancyManager.create_event_async`, which computes the epochs and validates them. The same code shape is behind the layering finding P2-1. **P1-3. Approving a month marks every day IMPORTED, including locally lived days, which then leave calibration history.** CONFIRMED (probe) `app/db/flow_history_repository.py:1298-1331` upserts `occupancy_business_day_schedules.source='IMPORTED'` for every day in the month. `occupancy_repository.py:3046-3048` / `3476-3478` and `has_imported_or_mixed_flow_async` (3279) exclude any day whose record is IMPORTED, whatever the day's flow source. Failure: 2026-08 was lived locally, and an admin approves it to fill a few gaps. All 31 days get `source=IMPORTED`. Their existing trusted calibration logs disappear from EWMA/variance/sample-history selection, so the next nightly k changes. The probe showed a day with only local hours (approved `source='local_events'`): its schedule became IMPORTED and `has_imported_or_mixed_flow_async` returned True. The upsert also overwrites the current record (WEEKLY/EXCEPTION/MANUAL) of lived days with draft values, outside the correction and next-day-activation policy (ADR 0007). This contradicts #164 ("do not rewrite already-applied calibration"; the exclusion covers only imported or mixed days) and ADR 0008 (only imported days are "marked as imported"; a lived day's record is its own). #### P2 **P2-1. Layering: the repository holds domain logic, calls back into the service, and writes another aggregate's tables.** CONFIRMED - `flow_history_repository.py:1103-1118`: `get_curation_draft_async` and `approve_curation_draft_async` import `flow_history_service` from inside the repository, a circular, inverted dependency. Only tests use them (test_flow_history_publication.py:310, 354). code-standards §1.1 says repositories only persist. - `store_approved_month_async` (1120-1479) holds domain rules: day status tallies, day-source classification, the open-window computation and the whole quality verdict (1200-1221). It also hand-writes occupancy schedules, schedule audit, events and event audit, duplicating the `OccupancyRepository`/`OccupancyManager` writers (Shotgun Surgery, Duplicated Code). That duplication is how P1-2 happened. - Fix: the service computes `ApprovedDayRecord`s and the quality verdict. The repository receives typed records and persists them in one transaction, using shared connection-level helpers from the occupancy repository for schedule and event writes. **P2-2. Approval atomicity: every check runs outside the write transaction, and the final UPDATE has no guard.** PLAUSIBLE (race; the code path is confirmed by reading) `flow_history_service.py:1246-1310` reads the draft and all 31 day details, then calls `store_approved_month_async`, which only starts `BEGIN IMMEDIATE` at repo:1136. Inside the transaction nothing re-checks the draft's status or `revision`, and `UPDATE ... SET status='APPROVED' WHERE month=?` (1433-1440) has no status predicate and no rowcount check. Failure: admin A approves while admin B discards, or patches a day back to UNREVIEWED, in between. The publication is built from the stale read, and the DISCARDED draft is resurrected as APPROVED with its curation_days already deleted. Also, `CurationApproveRequest` has no `expected_revision` field, although every other #170 mutation takes one. Fix: re-read the draft row under `BEGIN IMMEDIATE`, require the expected status and revision, and raise `DraftConflictError` (409) on mismatch. **P2-3. Approval skips the PREPARED_FOR_APPROVAL lifecycle, and APPROVED drafts stay editable.** CONFIRMED - `flow_history_service.py:1247` rejects only DISCARDED, so a plain `DRAFT` can be approved directly (the probe printed "status before approve: DRAFT"; no test calls prepare-approval). The approve gates (1259-1280) duplicate prepare's gates (1149-1219) and have drifted from them. For example, approval does not re-check `camera_mismatch_reason` text, and it has no reason gate tied to `unresolved_camera_hours` or `suggested_status`. - #171 adds `APPROVED`, but the #170 mutation guards (service 831/907/960/1087; repo 605-607/655-657/721-723/804-806) block only `PREPARED_FOR_APPROVAL`. An APPROVED draft therefore accepts PATCH day/schedule/zero-confirm edits and drifts from what was published. - Fix: require `status == 'PREPARED_FOR_APPROVAL'` (checked in the transaction, see P2-2), rely on prepare's gates, and treat APPROVED as immutable, like PREPARED. **P2-4. Migration: the status/action CHECK changes in CREATE TABLE IF NOT EXISTS never reach an existing database.** CONFIRMED (probe) `app/db/database.py:866, 933`. Probe: initialise the DB with the pr/170 tree, then with pr/171. `UPDATE ... status='APPROVED'` fails with `CHECK constraint failed: status IN ('DRAFT','PREPARED_FOR_APPROVAL','DISCARDED')`, and the `'APPROVE'` audit insert fails the same way. Failure: if #170 merges and deploys before #171 (they are separate PRs), every approval on that DB returns 500 permanently. Fix: move the `APPROVED`/`APPROVE` values into #170 before it merges, or add a table-rebuild migration (#228 introduces a generic CHECK-rebuild helper). **P2-5. Quality thresholds ignore admin configuration; an empty COMPLETE day is trusted.** CONFIRMED (probe for the second part) - `flow_history_service.py:1282-1286`: `getattr(cfg, "trust_ratio_min", 0.80)` etc. run on the dict returned by `get_config_async`, so they always return the hard-coded defaults. occupancy_service.py:2085-2165 correctly uses `cfg.get(...)`. The same `getattr(cfg, "holiday_open_time")` bug exists in #170 (service ~993); report it to #170. - `flow_history_repository.py:1206`: when `total_vol == 0`, every check is skipped and `is_trusted=1`. Probe: a day with no data, all hours `NONE`, marked COMPLETE with a reason, was published `is_trusted=True` with 0 counts. #164 says "Admin coverage approval does not itself assert sensor trust". - `:1220`: `min(6, trust_min_active_hours)` silently caps the configured minimum at 6 (a magic number). **P2-6. Re-approval destroys the prior published snapshot.** CONFIRMED (reading) `flow_history_repository.py:1139-1144` deletes the previous `flow_history_approved_days`/`_hours`, and the months row is upserted in place. #164 requires that re-approval "replaces the published revision atomically while retaining prior snapshots". The test at test_flow_history_publication.py:531-543 checks only the staging rows. This overlaps the spec axis. **P2-7. Partly observed hours lose their unresolved state in publication, and open-window counts disagree with day totals.** CONFIRMED (reading) - `database.py:975-987` gives `flow_history_approved_hours` no unresolved flag. The repository (1262-1268) writes partly observed LOCAL hours as `source='local_events'` with their partial counts, which cannot be told apart from wholly observed hours. #164: "Leave partially observed hours unresolved". The cam02 case in the precedence test is seeded but never asserted. - `:1191-1198`: `open_window_enter` sums `enter_num` over all hours, including unresolved partly observed ones, while `enter_total` excludes them. So `open_window_enter` can exceed `enter_total`. The open-window computation also truncates minutes (open/close `"09:30"`/`"21:30"` count by hour only), returns 0 when the close time is past midnight, and parses the hour by slicing `hour_start[11:13]` (Primitive Obsession). **P2-8. Local rollups count data that published KPIs exclude.** CONFIRMED (reading) `occupancy_repository.py:3206-3226` sums every `people_counting_events` row, including reconstructed gap slices (`raw_payload.reconstructed`) and excluded or inactive cameras. The KPI and trust queries filter both out (`_COUNTED_CAMERA_JOIN/FILTER`, 3355-3358). Failure: year-over-year comparisons built on rollups older than 180 days are inflated relative to live statistics for the same period. Related, P3-level, PLAUSIBLE: the business day is attributed with the current `daily_reset_time` instead of the effective-dated reset (ADR 0007). **P2-9. The new test module bypasses `isolated_repository_db`.** CONFIRMED (reading) `tests/test_flow_history_publication.py:43-67` uses an autouse fixture that DELETEs 20 tables in the session-wide DB, including `occupancy_calibration_logs`, `occupancy_events` and `occupancy_business_day_schedules`, which other modules rely on. Its sibling `test_flow_history_curation.py:40` uses `pytestmark = usefixtures("isolated_repository_db")`. #233 and 1885b27 on master were exactly this class of order dependence. #### P3 1. **Validation:** `CurationApproveRequest` (schemas/flow_history.py:427-428) lacks the strip validator that every sibling request has. The probe stored reason `' x '`. 2. **Dead code / Speculative Generality:** - `draft.status == "DISCARDED"` (service:1247) can never be true, because `get_curation_draft_row_async` returns None for DISCARDED, which yields a 404. - `FlowHistoryRepository.has_imported_or_mixed_flow_async` (1591-1616) duplicates the occupancy one and is unused. The occupancy copy (3279-3304) hard-codes `'hikcentral_artemis_hourly'` instead of `HISTORICAL_FLOW_SOURCE`. - `FlowHistoryService.get_approved_day_async` (1318) has no route. - The `reason or "Historical month approval"` fallbacks (1361, 1377, 1426) can never fire. 3. **Typing:** - `day_details: list[Any]` (repo:1124) should be `list[CurationDayDetail]`. - `get_curation_draft_async` (repo:1103) has no return annotation. 4. **Data Clump:** the five `trust_*` parameters plus `now` travel together into a 12-argument `store_approved_month_async`. Bundle the thresholds into one type, or read them inside the service-side quality function. 5. **Mysterious Name:** `events_rolled_up` (occupancy_repository.py:3317) counts camera-hour groups, not events (the test expects 1 for 2 events). 6. **Event re-approval:** dedupe by `(start_date, name)` (repo:1386-1391) means changed event times on re-approval are silently ignored. 7. **N+1:** approval calls `get_curation_day_detail_async` once per day (service:1289-1296), and each call re-queries the whole month. #170 batched these queries for the month view. 8. **Test gaps:** - `GET /months/{month}/approved` has no test, although code-standards §4 requires one for every endpoint. - Unasserted responses in the setup steps: test lines 225-228, 261-268, 498-500, 523-525, 662-664, 708-710. - `fetch_id` is unused at line 197. - The rollback test asserts only the month's `reason`; it should also check that the day and hour rows are intact. It also goes through the test-only repository seam rather than the route. - Missing regressions: the P1-1 loop, timed events (P1-2), local days staying calibration-eligible (P1-3), concurrency or stale revision (P2-2), approve-from-DRAFT refused (P2-3), and partly observed hours (P2-7). 9. **Docs:** - CONTEXT.md does not define the new domain terms: approved historical flow, local rollup, hourly-aggregate quality result, day source `mixed`. - The schema comment at database.py:944-946 says the tables are "Consumed by read-only statistics deck and baselines", but nothing consumes them yet. 10. **PR hygiene (git-and-workflow):** the PR body is stale. It still says "Only a placeholder commit ... implementation has not started", and the Verification and Checklist boxes are unchecked although the 08:54 comment reports them done. The "Architectural impact (planned)" section should describe what was built. #### Notes: master and stack - No textual conflict with master: the merge base is 9a608c1. - Semantic clash with open PR #228 (same milestone). It adds an explicit `original_value` column and states that imported days "remain unfixed until approval". #171 fixes the Original Schedule only through a `STAMP` audit row (repo:1350-1365), which is the option ADR 0008 rejected. The test at line 305-307 depends on the audit-row reader. Whichever PR lands second must write `original_value` at approval and drop the audit-row dependency. - #170 carries the same `getattr(cfg, ...)`-on-dict bug for the holiday hours (see P2-5) and should own the CHECK values (see P2-4). ### Spec axis Spec: #164, the umbrella and decisions in #163, CONTEXT.md, ADR 0006, 0007 and 0008. Unless stated otherwise, line numbers refer to `app/db/flow_history_repository.py` at 5742d63. Probes ran in a git-archive export with the suite's temp DB. The probe file was deleted afterwards. **Verification:** these files pass: - `tests/test_flow_history_publication.py` (9 tests) - `tests/test_flow_history_curation.py` (17 tests) - `tests/test_calibration_{audit_trust,honesty,reconciliation}.py` `ruff check` and `ruff format --check` are clean. #### r53 ownership check - **Tables:** #170 (8c78911) has no `flow_history_approved_*` table. #171 now creates and writes all three (`app/db/database.py:946-995`). Done. - **Approved-revision diff:** this was dropped, not moved. Nothing in #171 compares a draft with the previously approved revision (see Spec P2-1). #### P1 **P1-1. Approval always fails on a database that already ran #170's schema.** CONFIRMED. - **Where:** `app/db/database.py:866` and `:933`. - **Cause:** #171 widens two CHECK constraints inside `CREATE TABLE IF NOT EXISTS`: - `flow_history_curation_drafts.status` gains `APPROVED`; - `flow_history_curation_audit.action` gains `APPROVE`. `IF NOT EXISTS` never changes an existing table, and there is no migration. - **Probe:** I ran `init_schema` from the #170 tree, then from the #171 tree. Afterwards, `UPDATE ... SET status='APPROVED'` failed with `CHECK constraint failed: status IN ('DRAFT','PREPARED_FOR_APPROVAL','DISCARDED')`, and the `APPROVE` audit insert failed the same way. - **Failure scenario:** #170 merges and deploys, then #171 deploys. Every `POST /approve` rolls back with a 500, so no month can ever be published. - **Fix:** either put both CHECK values in #170, or add a table-rebuild migration in #171. Test it with an upgrade from an existing database. **P1-2. Re-approval destroys the previously published revision.** CONFIRMED. - **Where:** lines 1139-1144, plus `create_curation_draft_record_async` at ~495-503. - **Spec:** "Re-approval … replaces the published revision atomically while retaining prior snapshots and actor/time/reason audit records." - **Cause:** the approval deletes every `flow_history_approved_days` and `flow_history_approved_hours` row for the month and upserts the single `flow_history_approved_months` row. Creating the revision-2 draft has already deleted the revision-1 `flow_history_curation_days`, which hold the day reviews and reasons. - **Probe:** I approved revision 1 with day reason "R1 reason", then fetched revision 2, drafted, reviewed and approved it. The result was 0 rows with "R1 reason", only the revision-2 hours, and 1 month row. Revision 1's day statuses, reasons, provenance, local-versus-HikCentral resolution and hours are gone. What remains: - the staged HikCentral rows; - one APPROVE audit row holding month totals. - **Why the test misses it:** `test_repeated_approval_replaces_published_revision` checks only staging rows and the count of APPROVE audit rows. - **Fix:** keep published revisions, for example keyed by `(month, published_revision)` with a current pointer, or move superseded rows into history tables. The prior snapshot must stay queryable. #### P2 **P2-1. The "differences from a previously approved revision" view is gone.** CONFIRMED (code read). - **Spec:** #163 acceptance asks the review to show differences from a previously approved revision. - **What happened:** r53 (P2-10) asked for the diff to move into #171 next to its writer. Neither #170 nor #171 has it now. No field in the schemas compares a draft with the approved tables (`git grep` for approved, previous or diff in `app/schemas/flow_history.py` and `app/services/flow_history_service.py`). - **Failure scenario:** an admin re-reviews August after a HikCentral revision and cannot see which days or counts changed from what is published. - **Fix:** add the diff in #171, against the retained prior snapshot from P1-2. It should compare status, enter and exit totals, and source. Add a test. **P2-2. Approval skips the prepare step and does not lock the published draft.** CONFIRMED. - **Where:** `app/services/flow_history_service.py:1247` checks only `DISCARDED`. The update guards at 831, 907, 960 and 1087 reject only `PREPARED_FOR_APPROVAL`. - **Probes:** - (a) A `DRAFT` that was never prepared approved with a 200, so #170's prepare gates, its reason and its `PREPARE_APPROVAL` audit are optional. - (b) Approving the same draft twice gave 200 and 200. It republished the same revision, wrote a second APPROVE audit, and doubled the schedule audit rows from 31 to 62 (31 extra `OVERWRITE` rows). - (c) After approval, `PATCH /draft/days/{d}` returned 200 on the `APPROVED` draft. The draft and the published month now disagree, and another approve would publish the edit without preparation. - **Spec:** #164 says "Require … validated day reviews". It also asks to "Test … repeated approval". - **Fix:** - Approve only `PREPARED_FOR_APPROVAL` drafts. - Make an approval of an already-approved draft an idempotent no-op or a 409. - Treat `APPROVED` as immutable on every edit path. **P2-3. Approval reads the draft outside its transaction and never re-checks it.** CONFIRMED. - **Where:** the draft and day details are built at `app/services/flow_history_service.py:1245-1296`, before `BEGIN IMMEDIATE` (line 1136). Step 3 (line 1432) updates the draft with no status or revision predicate. - **Probe:** I made `store_approved_month_async` discard the draft just before it ran. Approval still published all 31 days and flipped the `DISCARDED` draft to `APPROVED`. Its curation days had already been deleted. - **The same window allows two more failures:** - an edit landing between the read and the write gets published stale; - two concurrent approvals both succeed. - **Fix:** inside the transaction, re-read the draft row. Require `status = 'PREPARED_FOR_APPROVAL'` and the expected `revision`, or use `UPDATE … WHERE status=? AND revision=?` and check `rowcount`. **P2-4. Approval rewrites the business-day record of locally lived days as `IMPORTED`.** CONFIRMED. - **Where:** lines 1304-1331. The approval upserts `source='IMPORTED'` for every day of the month and recomputes `is_exception` and `exception_name` from the draft's `day_type`. - **Probe:** I seeded a lived day with an `EXCEPTION` record ("Aniversario", `is_exception=1`), a FREEZE audit row, and local events in every camera-hour. After approval: - the approved day `source` was `local_events`; - the business-day record `source` was `IMPORTED`, with `is_exception=0` and `exception_name=None`; - `has_imported_or_mixed_flow_async` returned `True`. - **Consequences:** 1. A purely local day leaves calibration history. Both history queries filter `source = 'IMPORTED'` (`app/db/occupancy_repository.py:3047` and `:3477`), and the nightly path filters through `has_imported_or_mixed_flow_async`. The spec excludes only imported-or-mixed days. Publication should not change the calibration eligibility of a lived day. 2. The dated exception's identity is erased from the record that #178 makes authoritative. ADR 0008 marks only imported days as imported. - **Fix:** write or overwrite the record only for days without a lived record, or only for days whose source is imported or mixed. Never re-source a lived day. Keep `is_exception` and `exception_name` from the existing record. **P2-5. Local precedence is lost once raw events are pruned, and the local rollups are never read.** CONFIRMED. - **Where:** `get_month_local_events_async` (~985) still reads only `people_counting_events`. Nothing in `app/` reads `flow_history_local_*_rollups`. - **Probe:** month 2026-03, which is more than 180 days old. cam01 had a local event of 99 at 12:00, and HikCentral had staged 7 for the same hour. Before pruning, the curation hour was `LOCAL 99`. After `prune_retention_data_async`, it became `HIKCENTRAL 7`. - **Failure scenario:** an older month is approved after pruning. Imported counts then overwrite camera-hours that were observed locally, against the "local observations own any observed camera-hour" invariant. - **Note:** #164 also says the rollups exist "so year-over-year comparisons survive", but no consumer or read path ships. The #160 statistics child can consume them, but curation precedence belongs here. - **Fix:** have curation's local evidence take the union of raw events and hourly rollups. The rollup needs to mark the camera-hour as observed, including zero counts. **P2-6. The rollups count reconstructed estimates and cameras that are not counted.** CONFIRMED. - **Where:** `app/db/occupancy_repository.py:3206-3221`. - **Cause:** the rollup query sums every `people_counting_events` row. It has no `_COUNTED_CAMERA_FILTER` or join, and no `reconstructed` exclusion. The published KPIs and `get_hourly_flow_distribution_async` apply both. - **Probe:** a real event of 99, a reconstructed slice of 500, and 300 from an unregistered `camXX` produced a daily rollup of 899. Every row was labelled `source='local_events'`. - **Failure scenario:** a year-over-year comparison reads a day at about nine times its counted traffic, and gap estimates pass for observations. - **Fix:** apply the same counted-camera and non-reconstructed filters. Store reconstructed volume separately if it is needed at all. **P2-7. The quality result trusts a Complete day that has no counts.** CONFIRMED. - **Where:** lines 1200-1221. - **Cause:** when `total_vol == 0`, every check is skipped and `is_trusted` stays 1. - **Probe:** with nothing staged, every day had 288 unresolved hours. All days were set to COMPLETE with a reason, then approved. The result: `status=COMPLETE`, `enter_total=0`, `is_trusted=True`, the day `source` was the HikCentral source, and all 288 hours had `source='NONE'`. - **Spec:** - "Admin coverage approval does not itself assert sensor trust." - "A Complete imported day can be eligible … after quality checks." Here the admin's coverage assertion alone produces a trusted, comparison-eligible empty day. - **Fix:** zero volume, and any `NONE` hour on a Complete day, should give an untrusted result with a flag. **P2-8. The independent quality result is weaker than the live R1-R4 checks and ignores the admin's configuration.** CONFIRMED (code read). - **Configuration ignored:** `app/services/flow_history_service.py:1282-1286` uses `getattr(cfg, …)` on the dict returned by `get_config_async`, so the configured trust thresholds are always replaced by the hard-coded defaults. - **Threshold clamped:** line 1220 uses `min(6, trust_min_active_hours)`, which silently lowers R1 from 10 to 6. - **Checks missing:** there is no minimum-footfall check (R4) and no per-hour gap awareness. - **No flags stored:** only a boolean is stored, so no one can see why a day is untrusted. - **Fix:** use `cfg.get(...)` and the live thresholds. Store the flags with the result. **P2-9. A re-approval leaves stale event annotations.** PLAUSIBLE (code read, not probed). - **Where:** lines 1385-1430. - **Cause:** events are inserted only if a matching `(start_date, name)` is missing, and nothing removes them. - **Failure scenario:** revision 1 annotates "Concierto" on 08-14. Revision 2 removes or renames it. The old event stays in `occupancy_events`, so the day remains an Event Day for statistics and is excluded from learning. A changed annotation creates a second event. - **Spec:** "replaces the published revision atomically." - **Fix:** track the events created by an approval, by approval revision. Replace or retire them on re-approval, with an audit entry. #### P3 1. **A Missing day is labelled as HikCentral data** (line 1186). A day whose hours are all `NONE` gets the HikCentral source. That is misleading provenance, and it also triggers the calibration exclusion. A neutral value such as `none` would be clearer. 2. **The rollups use today's reset time** (`app/db/occupancy_repository.py:3244`). `active_business_day(h_epoch, reset_time)` takes the current `daily_reset_time`, not the ADR 0007 effective-dated reset (`reset_for(day)`). After a reset-time change, old hours are assigned to the wrong business day. Hourly buckets also straddle a reset that is not on the hour. 3. **Open-window counts truncate opening hours** (lines 1189-1198). `open_window_enter` uses whole hours only, so 10:30 becomes 10:00. It returns 0 for a close time after midnight. 4. **The nocturnal-audit skip also drops local data** (`app/services/occupancy_service.py:2498`). It skips mixed days and lived days re-sourced by P2-4. The spec forbids an audit "solely from imported aggregates", so a mixed day's local data could still be audited. Mostly fixed by P2-4. 5. **Legacy calibration logs can vanish from history** (`app/db/occupancy_repository.py:3049-3052`). The new `cycle_date NOT IN (subquery)` clause drops logs whose `cycle_date` is NULL once any approved day exists, because NULL NOT IN a non-empty set is not true. PLAUSIBLE, since the holiday clause already behaves this way. 6. **The repository calls the service** (lines 1103-1118). `get_curation_draft_async` and `approve_curation_draft_async` in the repository import and call the service, which inverts the layering. The rollback test depends on these shims. 7. **The approved-day read has no route.** `get_approved_day_async` exists in the service, but no route exposes it. Hour-level approved data is unreachable over the API. 8. **The Original Schedule still comes from the oldest audit row.** Approval writes it through a STAMP audit row (the comment cites ADR 0008), but ADR 0008, which is accepted, rejected exactly that inference. This is fine for now, because master has no stored column yet. Note it in the PR as owed to #186 item 7. 9. **The PR body is stale.** It says "Only a placeholder commit … implementation has not started", lists the architecture as planned, and has unchecked verification. It also does not mention the new approve and approved routes, or that the approval writes business-day records and events. 10. **Missing tests:** - approval from a DRAFT that was never prepared; - double approval; - editing an approved draft; - concurrent discard or edit during approval; - upgrade from an existing #170 schema; - local precedence after pruning; - rollup filters; - a zero-volume Complete day; - a lived day inside an approved month; - re-approval with a changed event annotation; - retention of the prior snapshot. 11. **CONTEXT.md has no glossary entries** for the approved hourly aggregate, the local rollup, or a mixed-source day. #### Master - The merge base is master 9a608c1, and master has not changed any of these files since. I expect no textual conflicts. - Semantic risk: P1-1, if #170 deploys on its own.
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feat/historical-flow-publication:feat/historical-flow-publication
git switch feat/historical-flow-publication

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch feat/historical-flow-curation
git merge --no-ff feat/historical-flow-publication
git switch feat/historical-flow-publication
git rebase feat/historical-flow-curation
git switch feat/historical-flow-curation
git merge --ff-only feat/historical-flow-publication
git switch feat/historical-flow-publication
git rebase feat/historical-flow-curation
git switch feat/historical-flow-curation
git merge --no-ff feat/historical-flow-publication
git switch feat/historical-flow-curation
git merge --squash feat/historical-flow-publication
git switch feat/historical-flow-curation
git merge --ff-only feat/historical-flow-publication
git switch feat/historical-flow-curation
git merge feat/historical-flow-publication
git push origin feat/historical-flow-curation
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
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gabogg/hikcentral!171
No description provided.