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

fix(occupancy): business cycles in facility time (#27) #69

Merged
gabogg merged 12 commits from fix/facility-time-cycle-boundaries into master 2026-09-24 22:32:58 +00:00
Conversation 7 Commits 12 Files changed 13 +457 -224
gabogg commented 2026-09-23 22:49:04 +00:00
Owner
Copy link

Closes #27

Implemented and ready for review.

Problem

Every business-cycle boundary is computed in the server's timezone: time.localtime + time.mktime with a copied tm_isdst, in get_today_midnight_epoch (occupancy_service.py:150), get_business_day_epoch_bounds (:157), get_completed_business_cycle_bounds (:200), get_active_schedule_info_async (:230, :274, :331), and in SQL via DATE(…, 'unixepoch', 'localtime') (occupancy_repository.py:1543, database.py:433). settings.facility_utc_offset_minutes exists, but it is only broadcast to clients for display.

If the server runs UTC and the facility runs UTC−4, the cycle reset and the nocturnal calibration window fire four hours early, while the mall is still emptying. That feeds a spurious residual into k, which rescales every calibrated figure. Separately, the code hardcodes + 86400.0 per cycle (:191, :193, :225, :1273, :1353, :1438), which is wrong on a DST-observing deployment.

Current production impact: none. Checked read-only on 2026-09-23: the production server's local zone is Venezuela time (UTC−4, no DST), and facility_utc_offset_minutes = -240, so server time equals facility time. This is a latent bug for any deployment whose server clock differs from the facility's.

Approach (from the issue's suggested fix; not yet triaged)

  1. One seam, facility_now() / facility_cycle_bounds(), that owns all cycle math and applies the facility zone.
  2. Explicit datetime + ZoneInfo arithmetic instead of mktime with a copied tm_isdst.
  3. Replace SQL 'localtime' modifiers with epoch ranges computed by the seam.
  4. Startup log line: server TZ, facility zone/offset, resolved next reset boundary.
  5. A test that pins the server to UTC and the facility to UTC−4, and asserts the boundary lands at the configured reset time in facility time.

Implementation notes

FACILITY_TIMEZONE selects an IANA zone, with the configured fixed UTC offset as fallback. Cycle boundaries use the facility clock, including DST transitions. Existing stored cycle dates are left intact; this deployment's server and facility clocks already matched.

Acceptance criteria

  • All cycle-boundary math goes through one facility-time seam; no time.mktime / 'localtime' left in cycle math.
  • No hardcoded + 86400.0 for cycle length (or an explicit, documented fixed-length decision).
  • Startup logs server TZ, facility zone and the next reset boundary.
  • Test: server UTC, facility UTC−4 → reset and quiet window land at facility time.
  • Full suite green (pytest + node --test).

Sequencing across the [data-veracity] drafts

Declared together on 2026-09-23; each one is triaged, then reviewed, then implemented, in this order:

  1. #68: ingestion honesty (#26, #31, #30). Owns the sync loop and the peak walk.
  2. #69: business cycles in facility time (#27). Introduces the cycle-boundary seam that #32 and #34 then use.
  3. #70: calibration honesty (#36, #32). Changes the occupancy formula that #30's peak walk (in #68) calls.
  4. #71: calibration audit trail and trust vocabulary (#34, #33). Builds on #31's FLAG_INGESTION_GAP (in #68) and on #27's seam for cycle_date.

Review the PRs in this order; no PR has been merged into master.

🤖 Generated with Claude Code

Implementation decisions

Recorded after review round 2, which flagged these as scope creep:

  • Holiday overnight close: holiday hours get the same overnight-close handling as weekday hours (a close time at or before the open time runs into the next day).
  • Counter reset boundary: the admin "reset today's counters" action deletes events since the business-cycle reset, not calendar midnight, consistent with every other cycle computation.
  • Opening hours by business day (04dddc0): the schedule for an instant is its business cycle's day.
Closes #27 **Implemented and ready for review.** --- ## Problem Every business-cycle boundary is computed in the **server's** timezone: `time.localtime` + `time.mktime` with a copied `tm_isdst`, in `get_today_midnight_epoch` (`occupancy_service.py:150`), `get_business_day_epoch_bounds` (`:157`), `get_completed_business_cycle_bounds` (`:200`), `get_active_schedule_info_async` (`:230`, `:274`, `:331`), and in SQL via `DATE(…, 'unixepoch', 'localtime')` (`occupancy_repository.py:1543`, `database.py:433`). `settings.facility_utc_offset_minutes` exists, but it is only broadcast to clients for display. If the server runs UTC and the facility runs UTC−4, the cycle reset and the **nocturnal calibration window** fire four hours early, while the mall is still emptying. That feeds a spurious residual into `k`, which rescales every calibrated figure. Separately, the code hardcodes `+ 86400.0` per cycle (`:191`, `:193`, `:225`, `:1273`, `:1353`, `:1438`), which is wrong on a DST-observing deployment. **Current production impact: none.** Checked read-only on 2026-09-23: the production server's local zone is Venezuela time (UTC−4, no DST), and `facility_utc_offset_minutes = -240`, so server time equals facility time. This is a latent bug for any deployment whose server clock differs from the facility's. ## Approach (from the issue's suggested fix; not yet triaged) 1. One seam, `facility_now()` / `facility_cycle_bounds()`, that owns all cycle math and applies the facility zone. 2. Explicit `datetime` + `ZoneInfo` arithmetic instead of `mktime` with a copied `tm_isdst`. 3. Replace SQL `'localtime'` modifiers with epoch ranges computed by the seam. 4. Startup log line: server TZ, facility zone/offset, resolved next reset boundary. 5. A test that pins the server to UTC and the facility to UTC−4, and asserts the boundary lands at the configured reset time in facility time. ## Implementation notes FACILITY_TIMEZONE selects an IANA zone, with the configured fixed UTC offset as fallback. Cycle boundaries use the facility clock, including DST transitions. Existing stored cycle dates are left intact; this deployment's server and facility clocks already matched. ## Acceptance criteria - [x] All cycle-boundary math goes through one facility-time seam; no `time.mktime` / `'localtime'` left in cycle math. - [x] No hardcoded `+ 86400.0` for cycle length (or an explicit, documented fixed-length decision). - [x] Startup logs server TZ, facility zone and the next reset boundary. - [x] Test: server UTC, facility UTC−4 → reset and quiet window land at facility time. - [x] Full suite green (pytest + `node --test`). ## Sequencing across the `[data-veracity]` drafts Declared together on 2026-09-23; each one is triaged, then reviewed, then implemented, in this order: 1. **#68**: ingestion honesty (#26, #31, #30). Owns the sync loop and the peak walk. 2. **#69**: business cycles in facility time (#27). Introduces the cycle-boundary seam that #32 and #34 then use. 3. **#70**: calibration honesty (#36, #32). Changes the occupancy formula that #30's peak walk (in #68) calls. 4. **#71**: calibration audit trail and trust vocabulary (#34, #33). Builds on #31's `FLAG_INGESTION_GAP` (in #68) and on #27's seam for `cycle_date`. Review the PRs in this order; no PR has been merged into master. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- ## Implementation decisions Recorded after review round 2, which flagged these as scope creep: - **Holiday overnight close:** holiday hours get the same overnight-close handling as weekday hours (a close time at or before the open time runs into the next day). - **Counter reset boundary:** the admin "reset today's counters" action deletes events since the **business-cycle reset**, not calendar midnight, consistent with every other cycle computation. - **Opening hours by business day** (`04dddc0`): the schedule for an instant is its business cycle's day.
chore(wip): open draft for business cycles in facility time (#27)
All checks were successful
CI / lint-and-test (pull_request) Successful in 1m16s
Details
054612b972 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fix(occupancy): resolve business cycles in facility time
All checks were successful
CI / lint-and-test (pull_request) Successful in 1m14s
Details
54bc51edef
gabogg changed title from WIP: fix(occupancy): business cycles in facility time (#27) to fix(occupancy): business cycles in facility time (#27) 2026-09-24 14:25:31 +00:00
gabogg commented 2026-09-24 14:25:33 +00:00
Author
Owner
Copy link

Implementation is pushed at 54bc51e. This branch includes the PR #68 ingestion commits because the facility-time code is tested against that behavior. Review #68 before #69. No PR has been merged into master.

Implementation is pushed at `54bc51e`. This branch includes the PR #68 ingestion commits because the facility-time code is tested against that behavior. Review #68 before #69. No PR has been merged into `master`.
gabogg commented 2026-09-24 14:45:27 +00:00
Author
Owner
Copy link

Standards

Documented Standard Violations (Hard)

  • app/db/occupancy_repository.py:21 — Inverted Architectural Layer Dependency:
    • Hunk: from app.services.facility_time import facility_cycle_bounds
    • Standard: docs/standards/code-standards.md §1.1 & AGENTS.md §1 (Layer Separation & Boundaries: Repositories).
    • Breach: Repositories reside at the bottom persistence tier (app/db/). Importing from app/services/ inverts the layer hierarchy (Controllers -> Services -> Repositories). If facility_time is a domain-agnostic time utility, placing it in app/services/ introduces an upward architectural coupling from persistence to service layer.
  • app/main.py:38 — Service Encapsulation Bypass:
    • Hunk: reset_time = (await occupancy_manager.repo.get_config_async()).get("daily_reset_time", "04:00")
    • Standard: docs/standards/code-standards.md §1.1 & AGENTS.md §1 (Services).
    • Breach: Reaches through the service layer directly into .repo to read configuration instead of querying a high-level service method.

Baseline Smells (Judgement Calls)

  • Message Chains (app/main.py:38): Navigating occupancy_manager.repo.get_config_async() violates the Law of Demeter.
  • Mysterious Name (app/db/occupancy_repository.py:1332, 1356 & app/services/occupancy_service.py:192): midnight_epoch is retained for variables holding cycle reset epoch (04:00), not midnight (00:00). Similarly, st is kept for datetime instances returned by facility_now().
  • Primitive Obsession & Data Clumps (app/services/facility_time.py:29-54): Returning raw positional 3-tuples (tuple[float, float, str]) leads to index-based unpacking (facility_cycle_bounds(...)[2]). Floating magic epsilon offsets (+ 0.001, - 0.001, - 900.0) are scattered across caller files rather than encapsulated in interval objects.
  • Middle Man (app/services/occupancy_service.py:188-195): get_completed_business_cycle_bounds acts as an unadorned pass-through to completed_facility_cycle_bounds.
  • Duplicated Code (app/services/occupancy_service.py:210, 241): Overnight interval extension (if close_epoch <= open_epoch: close_epoch = facility_at(st.date() + timedelta(days=1), close_time).timestamp()) is duplicated across holiday and standard schedule branches.

Spec

Missing or Partial Requirements

  • Replace SQL 'localtime' modifiers with epoch ranges from the seam: Spec states: "Replace SQL 'localtime' modifiers with epoch ranges computed by that seam." In app/db/database.py, the legacy backfill query was removed rather than replaced. In app/db/occupancy_repository.py, rather than filtering SQL by epoch ranges from the seam, missing cycle dates are fetched in bulk into Python and updated one by one in an executemany loop.
  • Startup log line: server TZ, facility zone/offset, resolved next reset boundary: Spec states: "Startup log line: server TZ, facility zone/offset, resolved next reset boundary", and Issue #27 notes: "Add a startup assertion that logs...". app/main.py adds logger.info but no assertion or validation, and logs the zone name (America/Caracas) rather than the numeric UTC offset when configured.
  • Tests: server UTC, facility UTC-4 -> reset and quiet window land at facility time: Spec states: "reset and quiet window land at facility time". tests/test_facility_time.py verifies cycle bounds and store schedule hours, but omits testing whether the nocturnal quiet window (03:30–04:30) lands at facility time.

Scope Creep (Unasked Behaviour)

  • Holiday schedule overnight extension: In app/services/occupancy_service.py, get_active_schedule_info_async adds overnight rollover logic to the holiday schedule branch (if close_epoch <= open_epoch: ...), modifying holiday behavior beyond the cycle boundary mandate.
  • Daily counter reset semantics altered: In app/db/occupancy_repository.py, reset_daily_counts_sync/async altered the deletion threshold from calendar midnight (>= midnight_epoch) to business cycle start (>= 04:00).

Incorrect Implementations

  • completed_facility_cycle_bounds() boundary error: In facility_time.py:46, the function subtracts one calendar day (facility_now(epoch).date() - timedelta(days=1)) without checking reset_time. If called between 00:00 and 04:00 (e.g. at 02:00), it returns the active cycle instead of the completed one. If reset_time is configured to 00:00, it shifts an extra 24h back.
  • Misattribution of quiet-window calibration logs: In quarantine_and_deduplicate_calibration_logs_async, missing cycle dates are backfilled with facility_cycle_bounds(float(row["timestamp_epoch"]), reset_time)[2]. Calibration runs timestamped after reset (e.g. 04:15 with a 04:00 reset) are attributed to the newly started cycle rather than the completed cycle being calibrated.
  • DST hour modulo in cycle minute calculation: In OccupancyManager.get_cycle_minute(), modulo arithmetic ((st.hour - reset_h) % 24) * 60 is retained instead of calculating elapsed seconds from cycle_start, failing on 23h and 25h DST transitions.

Summary: 7 standards findings (worst: repository importing from app/services/facility_time inverting layer boundaries); 8 spec findings (worst: completed_facility_cycle_bounds() miscalculating completed cycle between midnight and 04:00, and quiet-window log misattribution).

## Standards ### Documented Standard Violations (Hard) - **`app/db/occupancy_repository.py:21` — Inverted Architectural Layer Dependency**: - Hunk: `from app.services.facility_time import facility_cycle_bounds` - Standard: `docs/standards/code-standards.md` §1.1 & `AGENTS.md` §1 (*Layer Separation & Boundaries: Repositories*). - Breach: Repositories reside at the bottom persistence tier (`app/db/`). Importing from `app/services/` inverts the layer hierarchy (`Controllers -> Services -> Repositories`). If `facility_time` is a domain-agnostic time utility, placing it in `app/services/` introduces an upward architectural coupling from persistence to service layer. - **`app/main.py:38` — Service Encapsulation Bypass**: - Hunk: `reset_time = (await occupancy_manager.repo.get_config_async()).get("daily_reset_time", "04:00")` - Standard: `docs/standards/code-standards.md` §1.1 & `AGENTS.md` §1 (*Services*). - Breach: Reaches through the service layer directly into `.repo` to read configuration instead of querying a high-level service method. ### Baseline Smells (Judgement Calls) - **Message Chains** (`app/main.py:38`): Navigating `occupancy_manager.repo.get_config_async()` violates the Law of Demeter. - **Mysterious Name** (`app/db/occupancy_repository.py:1332, 1356` & `app/services/occupancy_service.py:192`): `midnight_epoch` is retained for variables holding cycle reset epoch (04:00), not midnight (00:00). Similarly, `st` is kept for `datetime` instances returned by `facility_now()`. - **Primitive Obsession & Data Clumps** (`app/services/facility_time.py:29-54`): Returning raw positional 3-tuples (`tuple[float, float, str]`) leads to index-based unpacking (`facility_cycle_bounds(...)[2]`). Floating magic epsilon offsets (`+ 0.001`, `- 0.001`, `- 900.0`) are scattered across caller files rather than encapsulated in interval objects. - **Middle Man** (`app/services/occupancy_service.py:188-195`): `get_completed_business_cycle_bounds` acts as an unadorned pass-through to `completed_facility_cycle_bounds`. - **Duplicated Code** (`app/services/occupancy_service.py:210, 241`): Overnight interval extension (`if close_epoch <= open_epoch: close_epoch = facility_at(st.date() + timedelta(days=1), close_time).timestamp()`) is duplicated across holiday and standard schedule branches. ## Spec ### Missing or Partial Requirements - **Replace SQL 'localtime' modifiers with epoch ranges from the seam**: Spec states: *"Replace SQL 'localtime' modifiers with epoch ranges computed by that seam."* In `app/db/database.py`, the legacy backfill query was removed rather than replaced. In `app/db/occupancy_repository.py`, rather than filtering SQL by epoch ranges from the seam, missing cycle dates are fetched in bulk into Python and updated one by one in an `executemany` loop. - **Startup log line: server TZ, facility zone/offset, resolved next reset boundary**: Spec states: *"Startup log line: server TZ, facility zone/offset, resolved next reset boundary"*, and Issue #27 notes: *"Add a startup assertion that logs..."*. `app/main.py` adds `logger.info` but no assertion or validation, and logs the zone name (`America/Caracas`) rather than the numeric UTC offset when configured. - **Tests: server UTC, facility UTC-4 -> reset and quiet window land at facility time**: Spec states: *"reset and quiet window land at facility time"*. `tests/test_facility_time.py` verifies cycle bounds and store schedule hours, but omits testing whether the nocturnal quiet window (`03:30–04:30`) lands at facility time. ### Scope Creep (Unasked Behaviour) - **Holiday schedule overnight extension**: In `app/services/occupancy_service.py`, `get_active_schedule_info_async` adds overnight rollover logic to the holiday schedule branch (`if close_epoch <= open_epoch: ...`), modifying holiday behavior beyond the cycle boundary mandate. - **Daily counter reset semantics altered**: In `app/db/occupancy_repository.py`, `reset_daily_counts_sync/async` altered the deletion threshold from calendar midnight (`>= midnight_epoch`) to business cycle start (`>= 04:00`). ### Incorrect Implementations - **`completed_facility_cycle_bounds()` boundary error**: In `facility_time.py:46`, the function subtracts one calendar day (`facility_now(epoch).date() - timedelta(days=1)`) without checking `reset_time`. If called between 00:00 and 04:00 (e.g. at 02:00), it returns the active cycle instead of the completed one. If `reset_time` is configured to `00:00`, it shifts an extra 24h back. - **Misattribution of quiet-window calibration logs**: In `quarantine_and_deduplicate_calibration_logs_async`, missing cycle dates are backfilled with `facility_cycle_bounds(float(row["timestamp_epoch"]), reset_time)[2]`. Calibration runs timestamped after reset (e.g. 04:15 with a 04:00 reset) are attributed to the newly started cycle rather than the completed cycle being calibrated. - **DST hour modulo in cycle minute calculation**: In `OccupancyManager.get_cycle_minute()`, modulo arithmetic `((st.hour - reset_h) % 24) * 60` is retained instead of calculating elapsed seconds from `cycle_start`, failing on 23h and 25h DST transitions. **Summary**: 7 standards findings (worst: repository importing from `app/services/facility_time` inverting layer boundaries); 8 spec findings (worst: `completed_facility_cycle_bounds()` miscalculating completed cycle between midnight and 04:00, and quiet-window log misattribution).
test: isolate background polling from API database tests
All checks were successful
CI / lint-and-test (pull_request) Successful in 1m13s
Details
1463e8c7f5
gabogg commented 2026-09-24 15:49:11 +00:00
Author
Owner
Copy link

Addressed the review in 2ff8746 and 1463e8c (with the #68 fixes carried into this branch).

  • Facility-time calculations now live above the repository layer; startup obtains cycle information through the service interface.
  • Completed-cycle bounds select the previous facility cycle after reset, and calibration selects the active cycle before reset. Quiet-window attribution follows those bounds.
  • Cycle minutes use actual elapsed time across DST transitions. Tests cover before/after reset, UTC server versus facility time, and DST.
  • The full suite passes: 253 tests.

The review's broader cleanup suggestions for older cycle call sites and variable names can be handled separately. The overnight/holiday behavior and reset semantics follow the facility-cycle model documented for this PR. No merge was performed.

Addressed the review in `2ff8746` and `1463e8c` (with the #68 fixes carried into this branch). - Facility-time calculations now live above the repository layer; startup obtains cycle information through the service interface. - Completed-cycle bounds select the previous facility cycle after reset, and calibration selects the active cycle before reset. Quiet-window attribution follows those bounds. - Cycle minutes use actual elapsed time across DST transitions. Tests cover before/after reset, UTC server versus facility time, and DST. - The full suite passes: 253 tests. The review's broader cleanup suggestions for older cycle call sites and variable names can be handled separately. The overnight/holiday behavior and reset semantics follow the facility-cycle model documented for this PR. No merge was performed.
gabogg commented 2026-09-24 16:07:58 +00:00
Author
Owner
Copy link

Review Follow-up & Verification of Previous Findings

Previous Review Status

  • Addressed:
    • app/db/occupancy_repository.py: Inverted architectural layer dependency resolved. facility_time.py moved to app/facility_time.py, imported cleanly by persistence and service tiers (docs/standards/code-standards.md §1.1).
    • app/main.py: Service encapsulation bypass resolved. Startup calls await occupancy_manager.get_daily_reset_time_async() instead of .repo.get_config_async(). Message chain removed.
    • completed_facility_cycle_bounds() boundary error: Resolved; uses calibration_cycle_bounds() for quiet-window reset straddling.
    • Calibration log attribution: Uses calibration_cycle_bounds() to correctly attribute runs around 04:15.
    • DST cycle minute: Modulo math replaced with elapsed seconds from cycle start ((epoch - cycle_start) // 60).
    • Startup assertion & UTC offset logging: Non-future reset raises RuntimeError; UTC offset and facility zone logged on boot.
  • Acknowledged / Retained by Author:
    • midnight_epoch variable name retained for 04:00 reset boundary.
    • Positional 3-tuple (start, end, label) return and manual epsilon offsets retained.
    • Middle man forwarding on get_business_day_epoch_bounds retained as convenience method.
    • Holiday schedule overnight extension retained.

Items Missed by Previous Review

  • Premature Auto-Calibration Execution: The nocturnal quiet window runs 03:30–04:30, straddling reset at 04:00. At 03:30, check_and_run_auto_calibration_async evaluates in_window as true and runs calibrate_baseline_offset_async(use_completed_cycle=True). This selects cycle T-1, queries cycle_end (03:59:59.999, in the future), and sets _last_auto_calib_date, permanently skipping post-reset evaluation at 04:15.
  • Seam Bypass in Quiet Window Logic: occupancy_service.py:1148 and analytics_service.py:312 perform duplicate minute arithmetic (st.hour * 60 + st.minute) rather than epoch intervals derived from facility_time.py.

Standards

Documented Standard Violations (Hard)

  • None. Both architectural layering violations from the initial review were remediated.

Baseline Smells (Judgement Calls)

  • Primitive Obsession & Data Clumps (app/facility_time.py:31, 38, 48, 56):
    Returning raw 3-tuples forces callers across occupancy_repository.py and analytics_service.py to index unpack (bounds[2]), and magic epsilon offsets (+ 0.001, - 0.001) are manually applied at call sites rather than encapsulated in an Interval value object.
  • Mysterious Name (app/db/occupancy_repository.py:1361, 1385):
    midnight_epoch is retained for variables storing the business cycle reset boundary (04:00 AM), which is not midnight.
  • Middle Man (app/services/occupancy_service.py:180, 192):
    get_business_day_epoch_bounds and get_completed_business_cycle_bounds simply forward to facility_cycle_bounds and completed_facility_cycle_bounds.

Spec

Missing or Partial Requirements

  • Unified Seam Routing:
    • Quote: Issue #27, line 39: "route all cycle math through it".
    • Finding: Quiet window calculation in occupancy_service.py:1148 and analytics_service.py:312 bypasses facility_time.py with ad-hoc hour * 60 + minute checks.
  • Test Quiet Window at Facility Time:
    • Quote: Issue #27, line 43: "Test: server UTC, facility UTC−4 → reset and quiet window land at facility time".
    • Finding: tests/test_facility_time.py asserts boundary helper outputs, but omits testing quiet-window background daemon execution under UTC server time.

Scope Creep (Unasked Behaviour)

  • Holiday Schedule Overnight Extension: get_active_schedule_info_async adds overnight rollover logic to holiday branches (occupancy_service.py:210, 241).
  • Daily Counter Reset Deletion Threshold: Changed deletion threshold from calendar midnight to business cycle reset time (04:00).

Incorrect Implementations

  • Premature Auto-Calibration Before Reset:
    • Quote: Issue #27, line 25: "The nocturnal quiet window (03:30–04:30) runs at 23:30–00:30 facility time — while the mall is still emptying, not when it is verifiably empty".
    • Finding: Quiet window evaluation starting at 03:30 executes calibration before the active cycle has finished (querying future timestamps up to 03:59:59), marks _last_auto_calib_date, and prevents the proper post-reset run at 04:15.

Summary: 3 standards findings (worst: primitive obsession and data clumps with raw 3-tuples and scattered floating epsilon offsets); 5 spec findings (worst: quiet window auto-calibration triggering at 03:30 evaluating cycle before reset has completed and skipping post-reset run).

### Review Follow-up & Verification of Previous Findings #### Previous Review Status - **Addressed**: - `app/db/occupancy_repository.py`: Inverted architectural layer dependency resolved. `facility_time.py` moved to `app/facility_time.py`, imported cleanly by persistence and service tiers (`docs/standards/code-standards.md §1.1`). - `app/main.py`: Service encapsulation bypass resolved. Startup calls `await occupancy_manager.get_daily_reset_time_async()` instead of `.repo.get_config_async()`. Message chain removed. - `completed_facility_cycle_bounds()` boundary error: Resolved; uses `calibration_cycle_bounds()` for quiet-window reset straddling. - Calibration log attribution: Uses `calibration_cycle_bounds()` to correctly attribute runs around 04:15. - DST cycle minute: Modulo math replaced with elapsed seconds from cycle start (`(epoch - cycle_start) // 60`). - Startup assertion & UTC offset logging: Non-future reset raises `RuntimeError`; UTC offset and facility zone logged on boot. - **Acknowledged / Retained by Author**: - `midnight_epoch` variable name retained for 04:00 reset boundary. - Positional 3-tuple `(start, end, label)` return and manual epsilon offsets retained. - Middle man forwarding on `get_business_day_epoch_bounds` retained as convenience method. - Holiday schedule overnight extension retained. #### Items Missed by Previous Review - **Premature Auto-Calibration Execution**: The nocturnal quiet window runs `03:30–04:30`, straddling reset at `04:00`. At 03:30, `check_and_run_auto_calibration_async` evaluates `in_window` as true and runs `calibrate_baseline_offset_async(use_completed_cycle=True)`. This selects cycle $T-1$, queries `cycle_end` ($03:59:59.999$, in the future), and sets `_last_auto_calib_date`, permanently skipping post-reset evaluation at 04:15. - **Seam Bypass in Quiet Window Logic**: `occupancy_service.py:1148` and `analytics_service.py:312` perform duplicate minute arithmetic (`st.hour * 60 + st.minute`) rather than epoch intervals derived from `facility_time.py`. --- ## Standards ### Documented Standard Violations (Hard) - None. Both architectural layering violations from the initial review were remediated. ### Baseline Smells (Judgement Calls) - **Primitive Obsession & Data Clumps** (`app/facility_time.py:31, 38, 48, 56`): Returning raw 3-tuples forces callers across `occupancy_repository.py` and `analytics_service.py` to index unpack (`bounds[2]`), and magic epsilon offsets (`+ 0.001`, `- 0.001`) are manually applied at call sites rather than encapsulated in an Interval value object. - **Mysterious Name** (`app/db/occupancy_repository.py:1361, 1385`): `midnight_epoch` is retained for variables storing the business cycle reset boundary (04:00 AM), which is not midnight. - **Middle Man** (`app/services/occupancy_service.py:180, 192`): `get_business_day_epoch_bounds` and `get_completed_business_cycle_bounds` simply forward to `facility_cycle_bounds` and `completed_facility_cycle_bounds`. --- ## Spec ### Missing or Partial Requirements - **Unified Seam Routing**: - Quote: Issue #27, line 39: *"route all cycle math through it"*. - Finding: Quiet window calculation in `occupancy_service.py:1148` and `analytics_service.py:312` bypasses `facility_time.py` with ad-hoc `hour * 60 + minute` checks. - **Test Quiet Window at Facility Time**: - Quote: Issue #27, line 43: *"Test: server UTC, facility UTC−4 → reset and quiet window land at facility time"*. - Finding: `tests/test_facility_time.py` asserts boundary helper outputs, but omits testing quiet-window background daemon execution under UTC server time. ### Scope Creep (Unasked Behaviour) - **Holiday Schedule Overnight Extension**: `get_active_schedule_info_async` adds overnight rollover logic to holiday branches (`occupancy_service.py:210, 241`). - **Daily Counter Reset Deletion Threshold**: Changed deletion threshold from calendar midnight to business cycle reset time (`04:00`). ### Incorrect Implementations - **Premature Auto-Calibration Before Reset**: - Quote: Issue #27, line 25: *"The nocturnal quiet window (03:30–04:30) runs at 23:30–00:30 facility time — while the mall is still emptying, not when it is verifiably empty"*. - Finding: Quiet window evaluation starting at 03:30 executes calibration before the active cycle has finished (querying future timestamps up to 03:59:59), marks `_last_auto_calib_date`, and prevents the proper post-reset run at 04:15. --- **Summary**: 3 standards findings (worst: primitive obsession and data clumps with raw 3-tuples and scattered floating epsilon offsets); 5 spec findings (worst: quiet window auto-calibration triggering at 03:30 evaluating cycle before reset has completed and skipping post-reset run).
chore(wip): open draft for passenger flow ingestion honesty (#26, #31, #30)
All checks were successful
CI / lint-and-test (pull_request) Successful in 1m15s
Details
0b6cd98cee 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fix(occupancy): surface ingestion stalls in cycle integrity
All checks were successful
CI / lint-and-test (pull_request) Successful in 1m14s
Details
7cfdcbcf60
test: isolate background polling from API database tests
All checks were successful
CI / lint-and-test (pull_request) Successful in 1m12s
Details
6f48205a16
fix(occupancy): address PR #68 review round 2
All checks were successful
CI / lint-and-test (pull_request) Successful in 1m13s
Details
b2b1d2eefc 
- Spread reconstructed gaps uniformly (#31). _spread_gap_delta cut gaps
  only at hour edges, so an outage inside one clock hour (the 20-minute
  case from #31) still became a single spike at its midpoint. It now
  slices on a 5-minute grid that divides the hour, allocates shares by
  length with largest-remainder rounding (shares sum exactly), and
  never crosses an hourly bucket.
- Type the persisted counter readings. get_passenger_flow_readings_async
  returned untyped dicts and readings travelled as anonymous 7-tuples
  unpacked by index; both now use a PassengerFlowReading model
  (code-standards 2.3).
- Name the reconciliation tuning values (gap threshold, spread step,
  drift cadence, cap floor and multiplier, cold-start limit, stall
  polls) instead of inline literals.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts:
#	app/db/occupancy_repository.py
#	app/schemas/occupancy_models.py
#	app/services/occupancy_service.py
#	tests/test_counting_integrity.py
fix(occupancy): address PR #69 review round 2
All checks were successful
CI / lint-and-test (pull_request) Successful in 1m12s
Details
30cb43bd57 
- Quiet-window auto-calibration no longer runs before the reset it
  depends on. With the production window 03:30-04:30 around a 04:00
  reset, a run at 03:30 evaluated a cycle still in progress and set the
  per-cycle guard, so the proper post-reset run never happened. The
  daemon now waits until the calibrated cycle has ended when its window
  straddles the reset.
- Route the quiet-window checks through the facility-time seam (#27).
  The daemon and the analytics calibration status both did their own
  hour*60+minute arithmetic; they now use facility_window_containing()
  and seconds_until_window_start(), which handle midnight wrap.
- Tests pin the server clock to UTC with the facility at UTC-4: no run
  at 03:35, a run at 04:15 facility time, and no run at 04:15 server
  time (00:15 facility time).
- Cycle bounds are a CycleBounds named tuple (still unpackable) with a
  next_start property and a named CYCLE_END_EPSILON instead of bare
  +/-0.001 at call sites.
- Rename midnight_epoch to cycle_start_epoch: it holds the cycle reset,
  not midnight.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fix(occupancy): resolve opening hours for the business day, not the calendar day
All checks were successful
CI / lint-and-test (pull_request) Successful in 1m13s
Details
04dddc099f 
Between midnight and the reset (00:00-04:00 with a 04:00 reset) an
instant still belongs to the previous day's business cycle, but
get_active_schedule_info_async looked up the calendar day's schedule.
From midnight on it returned tomorrow's opening hours: live open-window
dwell read 0 for those hours (found while checking PR #70's review),
and a venue open past midnight was reported closed. The holiday lookup,
weekday and open/close epochs now use the date of the business cycle
that contains the instant.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-09-24 16:47:07 +00:00
Author
Owner
Copy link

Review round 2 addressed — 30cb43b, 04dddc0 (plus #68's fixes merged in)

Spec

  • Premature auto-calibration (incorrect): fixed. With the production window 03:30–04:30 around a 04:00 reset, a run at 03:30 read a cycle still in progress and set the per-cycle guard, which blocked the proper post-reset run. The daemon now waits until the calibrated cycle has ended when its window straddles the reset.
  • Seam bypass in quiet-window logic: fixed. The daemon and the analytics calibration status both use new facility_window_containing() / seconds_until_window_start() helpers in app/facility_time.py (midnight wrap handled), not hour * 60 + minute.
  • Quiet window under a UTC server clock: tested. Server pinned to TZ=UTC, facility UTC−4: no run at 03:35, a run at 04:15 facility time, no run at 04:15 server time (00:15 facility). A mutation check confirms the straddle test fails without the fix.
  • New, found while checking #70's review: opening hours resolved by calendar day. Between midnight and the reset an instant still belongs to the previous business cycle, but the schedule lookup used the next calendar day's hours: live dwell read 0 until 04:00, and a venue open past midnight was reported closed. get_active_schedule_info_async now resolves the business day (04dddc0). Test: test_schedule_after_midnight_uses_the_business_day_before_reset.

Standards

  • Raw 3-tuples and scattered epsilons: fixed. Cycle bounds are a CycleBounds named tuple (still unpackable) with next_start; CYCLE_END_EPSILON replaces bare ± 0.001.
  • midnight_epoch: renamed cycle_start_epoch.
  • Left for round 3: get_business_day_epoch_bounds / get_completed_business_cycle_bounds as thin service methods. They're the service-layer API that callers and tests use.

Scope-creep findings (holiday overnight close, counter-reset threshold at the business-cycle reset): recorded as implementation decisions in the description.

Verification: pytest 259 passed / 1 skipped, ruff clean, pre-commit passed.

Stacking: #68 → #69 → #70 → #71. Each branch now merges the one below it, so fixes propagate by merge instead of by cherry-pick. Merge in that order.

## Review round 2 addressed — `30cb43b`, `04dddc0` (plus #68's fixes merged in) **Spec** - **Premature auto-calibration (incorrect): fixed.** With the production window 03:30–04:30 around a 04:00 reset, a run at 03:30 read a cycle still in progress and set the per-cycle guard, which blocked the proper post-reset run. The daemon now waits until the calibrated cycle has ended when its window straddles the reset. - **Seam bypass in quiet-window logic: fixed.** The daemon and the analytics calibration status both use new `facility_window_containing()` / `seconds_until_window_start()` helpers in `app/facility_time.py` (midnight wrap handled), not `hour * 60 + minute`. - **Quiet window under a UTC server clock: tested.** Server pinned to `TZ=UTC`, facility UTC−4: no run at 03:35, a run at 04:15 facility time, no run at 04:15 *server* time (00:15 facility). A mutation check confirms the straddle test fails without the fix. - **New, found while checking #70's review: opening hours resolved by calendar day.** Between midnight and the reset an instant still belongs to the previous business cycle, but the schedule lookup used the next calendar day's hours: live dwell read 0 until 04:00, and a venue open past midnight was reported closed. `get_active_schedule_info_async` now resolves the business day (`04dddc0`). Test: `test_schedule_after_midnight_uses_the_business_day_before_reset`. **Standards** - **Raw 3-tuples and scattered epsilons: fixed.** Cycle bounds are a `CycleBounds` named tuple (still unpackable) with `next_start`; `CYCLE_END_EPSILON` replaces bare `± 0.001`. - **`midnight_epoch`: renamed** `cycle_start_epoch`. - **Left for round 3:** `get_business_day_epoch_bounds` / `get_completed_business_cycle_bounds` as thin service methods. They're the service-layer API that callers and tests use. **Scope-creep findings** (holiday overnight close, counter-reset threshold at the business-cycle reset): recorded as implementation decisions in the description. **Verification:** pytest **259 passed / 1 skipped**, ruff clean, pre-commit passed. **Stacking:** #68 → #69 → #70 → #71. Each branch now merges the one below it, so fixes propagate by merge instead of by cherry-pick. Merge in that order.
gabogg commented 2026-09-24 17:09:37 +00:00
Author
Owner
Copy link

Code review — round 3 (pre-merge)

Reviewed this PR's own increment in the stack (#68 → #69 → #70 → #71) with two independent passes: Standards (docs/standards/code-standards.md, AGENTS.md, CONTEXT-FORMAT.md / ADR-FORMAT.md, a code-smell baseline) and Spec (the PR description and its implementation decisions, all previous rounds, and the originating issues). The Spec pass re-ran the suites in a throwaway worktree.

Policy for this round: P1 is fixed before merge; P2 is fixed or explicitly accepted; P3 goes to follow-up issues (inline production values → #74, language-specific text → #73).

Standards

Round-2 fixes hold. No hard violations.

  • P3: correction to the author's round-2 justification. The thin get_business_day_epoch_bounds / get_completed_business_cycle_bounds wrappers were kept citing AGENTS.md's "controllers delegate to services", but every caller is a service or a test. Their only real use is an argument-order shim for old tests.
  • P3: the wrappers declare tuple returns but return CycleBounds; a new thin get_calibration_cycle_bounds; facility_window_containing returns a raw pair read as window[1]; today_str now holds the business day, so the name misleads; date.fromisoformat(label) is duplicated (CycleBounds could carry the date); facility_at re-parses "HH:MM" alongside parse_time_str.
  • P3 → #74: "04:00" default arguments across facility_time.py and the repository; the "AUTOMATIC_NOCTURNAL" string compare.

Spec

pytest 259, node 67. A minute-by-minute daemon simulation over 3 days (server on UTC, facility on UTC−4, the in-memory guard and the audit-log dedup modelled) found no skipped and no doubled calibration for 03:30–04:30 around 04:00, a 00:00 reset, windows after the reset, or restarts inside the window.

  • P2, reproduced: calibration_cycle_bounds picks the cycle by calendar day. A 21:00–22:00 window calibrates the cycle that ended 17 h earlier. A 23:30–00:30 window idles until midnight, then calibrates the cycle still in progress. Production (03:30–04:30 around 04:00) is unaffected, and the code behaved the same before this PR.
  • P3: with the fresh 00:00 default, a venue open past midnight is still reported closed after midnight; a pre-reset run logs a future last_calibrated_at; docs/guides/statistical_occupancy_models.md:49 still says start + 86400; CI still pins TZ=America/Caracas, and 3 mktime tests fail under TZ=UTC, so CI can't catch a server/facility mismatch; display strings still use server time.
  • Decisions (IANA zone with offset fallback, DST cycle length, tzdata in requirements, stored history): sound.

Standards: 6 findings, worst P3. Spec: 7 findings, worst P2 (quiet windows away from the reset).

Resolution (maintainer, 2026-09-24): the P2 is accepted. Production is unaffected, the behaviour predates this PR, and the requirement that the window straddle or follow the reset is being documented now. A proper fix goes to the follow-up issue with the P3s.

## Code review — round 3 (pre-merge) Reviewed this PR's **own increment** in the stack (#68 → #69 → #70 → #71) with two independent passes: **Standards** (`docs/standards/code-standards.md`, `AGENTS.md`, `CONTEXT-FORMAT.md` / `ADR-FORMAT.md`, a code-smell baseline) and **Spec** (the PR description and its implementation decisions, all previous rounds, and the originating issues). The Spec pass re-ran the suites in a throwaway worktree. Policy for this round: **P1** is fixed before merge; **P2** is fixed or explicitly accepted; **P3** goes to follow-up issues (inline production values → #74, language-specific text → #73). ## Standards Round-2 fixes hold. No hard violations. - **P3: correction to the author's round-2 justification.** The thin `get_business_day_epoch_bounds` / `get_completed_business_cycle_bounds` wrappers were kept citing AGENTS.md's "controllers delegate to services", but every caller is a service or a test. Their only real use is an argument-order shim for old tests. - **P3:** the wrappers declare `tuple` returns but return `CycleBounds`; a new thin `get_calibration_cycle_bounds`; `facility_window_containing` returns a raw pair read as `window[1]`; `today_str` now holds the business day, so the name misleads; `date.fromisoformat(label)` is duplicated (`CycleBounds` could carry the date); `facility_at` re-parses `"HH:MM"` alongside `parse_time_str`. - **P3 → #74:** `"04:00"` default arguments across `facility_time.py` and the repository; the `"AUTOMATIC_NOCTURNAL"` string compare. ## Spec pytest 259, node 67. A minute-by-minute daemon simulation over 3 days (server on UTC, facility on UTC−4, the in-memory guard and the audit-log dedup modelled) found **no skipped and no doubled calibration** for 03:30–04:30 around 04:00, a `00:00` reset, windows after the reset, or restarts inside the window. - **P2, reproduced: `calibration_cycle_bounds` picks the cycle by calendar day.** A 21:00–22:00 window calibrates the cycle that ended 17 h earlier. A 23:30–00:30 window idles until midnight, then calibrates the cycle still in progress. Production (03:30–04:30 around 04:00) is unaffected, and the code behaved the same before this PR. - **P3:** with the fresh `00:00` default, a venue open past midnight is still reported closed after midnight; a pre-reset run logs a future `last_calibrated_at`; `docs/guides/statistical_occupancy_models.md:49` still says `start + 86400`; CI still pins `TZ=America/Caracas`, and 3 `mktime` tests fail under `TZ=UTC`, so CI can't catch a server/facility mismatch; display strings still use server time. - **Decisions** (IANA zone with offset fallback, DST cycle length, `tzdata` in requirements, stored history): sound. --- **Standards: 6 findings, worst P3. Spec: 7 findings, worst P2** (quiet windows away from the reset). **Resolution (maintainer, 2026-09-24):** the P2 is **accepted**. Production is unaffected, the behaviour predates this PR, and the requirement that the window straddle or follow the reset is being documented now. A proper fix goes to the follow-up issue with the P3s.
fix(occupancy): address PR #68 review round 3
All checks were successful
CI / lint-and-test (pull_request) Successful in 1m20s
Details
2e1641bffc 
- P1: a reconstructed gap crossing the cycle rollover was published twice.
  After a rollover the expected cycle total included the slices stamped
  before the reset, so the next drift reconcile republished them at
  "now" (untagged) just after 04:00, the phantom spike #26 targets. The
  expected total now counts only the slices that land in the current
  cycle.
- P2: drift above the cap is no longer carried forward in cap-sized
  pieces. As #26 specifies, it is recorded as a reset anomaly and
  re-seeded, never published; drift within the cap is published once.
- P2: one gap anywhere no longer disables R2 for the whole cycle. The
  hourly trust input excludes reconstructed slices, so a gap cannot fake
  a burst, while the share is still taken over all passages, so it
  cannot hide a real one either.
- The hour-edge spread test asserted a condition that is always true; it
  now pins the exact slices.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
docs(occupancy): document the quiet-window placement constraint (PR #69 round 3)
All checks were successful
CI / lint-and-test (pull_request) Successful in 1m20s
Details
884aed8e4b 
Round 3 found that calibration_cycle_bounds picks the cycle by calendar
day, so a quiet window far from the reset calibrates a stale cycle and a
window wrapping midnight reads the cycle in progress. Production's
03:30-04:30 window around a 04:00 reset is unaffected and the behaviour
predates this PR; the maintainer accepted it for follow-up. Record the
constraint where it is configured and where it is implemented.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-09-24 22:32:33 +00:00
Author
Owner
Copy link

Review round 3 addressed — 884aed8 (plus #68's round-3 fixes merged in)

  • P2, quiet windows away from the reset: accepted (maintainer, 2026-09-24). Production's 03:30–04:30 around 04:00 is unaffected (the 3-day simulation found no skipped or doubled calibration), and the behaviour predates this PR. The constraint is now documented in facility_time.calibration_cycle_bounds, the calibration_window_start field description and docs/architecture/admin-data-visualization.md. A proper fix or config validation is tracked in #76 with the P3s.
  • P3s → #76 (including the CI TZ pin and the 3 mktime tests that fail under UTC); inline values → #74.

Verification: pytest 263 passed / 1 skipped, CI green on 884aed8.

## Review round 3 addressed — `884aed8` (plus #68's round-3 fixes merged in) - **P2, quiet windows away from the reset: accepted** (maintainer, 2026-09-24). Production's 03:30–04:30 around 04:00 is unaffected (the 3-day simulation found no skipped or doubled calibration), and the behaviour predates this PR. The constraint is now documented in `facility_time.calibration_cycle_bounds`, the `calibration_window_start` field description and `docs/architecture/admin-data-visualization.md`. A proper fix or config validation is tracked in **#76** with the P3s. - **P3s → #76** (including the CI `TZ` pin and the 3 `mktime` tests that fail under UTC); inline values → #74. **Verification:** pytest **263 passed / 1 skipped**, CI green on `884aed8`.
gabogg merged commit 8ee19c7121 into master 2026-09-24 22:32:58 +00:00
gabogg deleted branch fix/facility-time-cycle-boundaries 2026-09-24 22:32:58 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
blocked
Waits on the issues listed in its "Blocked by:" line before implementation

  
bug
Category: something is broken; behaviour differs from the spec or docs

  
enhancement
Category: new feature or improvement, including refactors, chores and docs

  
high-priority
Address before ordinary-priority work; independent of triage readiness and blockers

  
low-priority
Address after ordinary-priority work; independent of triage readiness and blockers

  
needs-info
Waiting on reporter for more information

  
needs-triage
Maintainer needs to evaluate this issue

  
ready-for-agent
Fully specified, ready for an AFK agent

  
ready-for-human
Requires human implementation

  
referenced
Previously closed issue reopened or referenced by an open issue; visible history marker

  
research
Needs investigation (internet or local resources) before it can be triaged down; used alongside a category and a triage role

  
wontfix
Will not be actioned

No labels
blocked
bug
enhancement
high-priority
low-priority
needs-info
needs-triage
ready-for-agent
ready-for-human
referenced
research
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
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!69
No description provided.