refactor(occupancy): pass-2 P3 follow-ups from PR #170 (historical flow curation) #273

New issue

Open

opened 2026-10-03 18:12:49 +00:00 by gabogg · 0 comments

gabogg commented

2026-10-03 18:12:49 +00:00

Owner

Follow-ups from review pass 2 of PR #170. These are polish and test gaps and none blocks the M1 delivery, so this issue stays outside the milestone.

Code:

_make_hour_coverage (flow_history_service) is a Middle Man that only forwards 15 kwargs.
Dead code:
- the SPECIAL_EVENT branch (svc ~1015) can't be reached;
- an ALTER TABLE ... ADD COLUMN revision migration targets a table that never shipped;
- CurationAuditAction is unused.
Duplicated validation:
- the reason validator is copied 7 times in the schema and repeated in the service and the discard controller;
- the discard controller accepts both a body and ?reason= and uses the deprecated HTTP_422_UNPROCESSABLE_ENTITY;
- the prepared-draft and revision checks appear in both the service and the repository.
Untyped dict[str, Any] passes between layers (code-standards §2.3). SQL column names are built from updates.keys(); whitelist them.
Status and action strings are bare literals, and the audit action is a plain str.
The month view loads the whole month's raw events and rescans them per day; group them in SQL instead (plausible perf issue).
The new BEGIN IMMEDIATE paths have no explicit rollback(), unlike the module pattern.
Code comments carry review IDs ("P2-9:", "(P1 & P1-1)").
Editing a prepared draft returns 422 VALIDATION_ERROR, though it is a state conflict (409); tests accept either code.
_CLOCK_PATTERN is imported across modules though it is private, and validate_hours imports re inside the function.
Mysterious names: th, ar, dr, sr, zr.
Missing validation: open_time later than close_time, and an event ending before it starts.
Every day PATCH clears schedule_confirmed, even a status-only review; the spec only asks to reset it on context edits.
Retrieval errors match only the exact revision, so a NULL-revision retry never clears an earlier failure (plausible). Align with get_month_summary_async's "latest settled attempt per (day, camera)".
Gap detection skips stall anomalies (global group_code='*', and per-group "counter advanced without events") and uses today's camera groups (plausible).

Tests and docs:

Calls without a status-code assertion: tests/test_flow_history_curation.py lines 287, 292, 306, 330, 370, 431, 479, 606, 616, 621, 627, 688, 719, 860, 914, 946 (as of 8c78911).
The "concurrency" test is sequential. The "PARTIAL without reason" case hits the schema's min_length, not the evidence gate.
The docs/api/README.md labels ("Confirm Zero Flow", "Update Curation Weekly Schedule") don't match the route names.
The PR body lacks the Checklist and Architectural Impact sections (git-and-workflow §2.2).

Acceptance criteria

Each item above is fixed or explicitly declined with a reason.

Follow-ups from review pass 2 of PR #170. These are polish and test gaps and none blocks the M1 delivery, so this issue stays outside the milestone. Code: - `_make_hour_coverage` (flow_history_service) is a Middle Man that only forwards 15 kwargs. - Dead code: - the `SPECIAL_EVENT` branch (svc ~1015) can't be reached; - an `ALTER TABLE ... ADD COLUMN revision` migration targets a table that never shipped; - `CurationAuditAction` is unused. - Duplicated validation: - the reason validator is copied 7 times in the schema and repeated in the service and the discard controller; - the discard controller accepts both a body and `?reason=` and uses the deprecated `HTTP_422_UNPROCESSABLE_ENTITY`; - the prepared-draft and revision checks appear in both the service and the repository. - Untyped `dict[str, Any]` passes between layers (code-standards §2.3). SQL column names are built from `updates.keys()`; whitelist them. - Status and action strings are bare literals, and the audit `action` is a plain `str`. - The month view loads the whole month's raw events and rescans them per day; group them in SQL instead (plausible perf issue). - The new `BEGIN IMMEDIATE` paths have no explicit `rollback()`, unlike the module pattern. - Code comments carry review IDs ("P2-9:", "(P1 & P1-1)"). - Editing a prepared draft returns 422 `VALIDATION_ERROR`, though it is a state conflict (409); tests accept either code. - `_CLOCK_PATTERN` is imported across modules though it is private, and `validate_hours` imports `re` inside the function. - Mysterious names: `th`, `ar`, `dr`, `sr`, `zr`. - Missing validation: `open_time` later than `close_time`, and an event ending before it starts. - Every day PATCH clears `schedule_confirmed`, even a status-only review; the spec only asks to reset it on context edits. - Retrieval errors match only the exact revision, so a NULL-revision retry never clears an earlier failure (plausible). Align with `get_month_summary_async`'s "latest settled attempt per (day, camera)". - Gap detection skips `stall` anomalies (global `group_code='*'`, and per-group "counter advanced without events") and uses today's camera groups (plausible). Tests and docs: - Calls without a status-code assertion: tests/test_flow_history_curation.py lines 287, 292, 306, 330, 370, 431, 479, 606, 616, 621, 627, 688, 719, 860, 914, 946 (as of 8c78911). - The "concurrency" test is sequential. The "PARTIAL without reason" case hits the schema's `min_length`, not the evidence gate. - The `docs/api/README.md` labels ("Confirm Zero Flow", "Update Curation Weekly Schedule") don't match the route names. - The PR body lacks the Checklist and Architectural Impact sections (git-and-workflow §2.2). ## Acceptance criteria - [ ] Each item above is fixed or explicitly declined with a reason.

gabogg added the

ready-for-agent

low-priority

labels

2026-10-03 18:12:49 +00:00

gabogg referenced this issue

2026-10-03 20:34:11 +00:00

chore(occupancy): third-pass P3 follow-ups from PR #170 #276