chore(schedule): PR #176 second-pass P3 follow-ups (audit actions, freeze migration, default stamp span) #186

New issue

Open

opened 2026-09-29 15:45:45 +00:00 by gabogg · 1 comment

gabogg commented

2026-09-29 15:45:45 +00:00

Owner

These P3 findings from the second review pass on #176 (business-day schedule records, #113) were deferred. None of them blocked the merge.

Standards

The audit action list is still written twice. Pass 1 flagged this and it is only partly fixed. ScheduleSource now generates its DB CHECK, but the audit action values are still spelled out in two places: the Literal at app/schemas/occupancy_models.py:390 and a hardcoded CHECK at app/db/database.py:362. 'FREEZE' is also a bare string literal at app/db/occupancy_repository.py:865.
- Acceptance: one definition of the audit actions, from which the CHECK is generated as for ScheduleSource, and the repository uses it instead of literals.
Existing audit tables don't allow FREEZE. 'FREEZE' was added only to the CREATE TABLE IF NOT EXISTS CHECK (database.py:362). A database whose audit table was created by the pass-1 branch, such as the local validation copy of prod, rejects every automatic freeze with an IntegrityError inside the monitor. Prod is safe because the table is new there.
- Acceptance: either a migration that rebuilds the CHECK when FREEZE is missing, or a note in the local-validation docs to recreate the database.
The first-recorded schedule reports the wrong recorded flag. get_original_business_day_schedule_async (occupancy_repository.py:976) decodes an audit old_value that can be an unrecorded fallback (recorded: false) through BusinessDaySchedule.from_record, which forces recorded=True. Only calibration reads it today.
- Acceptance: the decoded value keeps its stored recorded flag, with a test.
The CHECK constraints are built with f-strings (database.py:344-351). There's no injection risk because the values come from a Literal, but others may copy the pattern.
- Acceptance: build the constraint through one small helper that validates its input, or document why this is safe.
Missing docstrings on three of the four new service methods (occupancy_service.py:192,205,209).
- Acceptance: each has a one-line docstring saying what it freezes or writes.

Spec

The default stamp stops at the last counted day. Spec (#113): "records can exist for any date, not only days with counted data". The default ends at the last counted event (occupancy_service.py:357-360), and the test asserts that counted+3 stays unrecorded (test :168). A closed or outage stretch after counting stopped, up to yesterday, therefore stays on the fallback, and a later weekly edit would reclassify it. The same applies to days the monitor missed while it was down, because it freezes only the current day (monitor_service.py:108-115).
- Acceptance: the default stamp covers every completed day up to active_day - 1, with the test updated. Alternatively, the maintainer decides to keep the counted span, and the help text says so.
"First recorded value" depends on the order of audit rows. Calibration reads a day's first audit row (occupancy_repository.py:976-992). A future IMPORTED writer that adds records without an audit row would make calibration follow whatever that record later becomes. An OVERWRITE never changes the calibration value, and that behavior is not documented. This needs a design decision: keep the audit-row invariant (documented in the docstring and enforced for every writer), or add an explicit original_value column.
- Acceptance: the chosen rule is documented, and every record writer, including a future importer, follows it.
Small behavior changes that were not asked for. Record them in #113, or confirm them with the maintainer:
- the monitor now re-reads the reset time only every 60 s or at the next boundary (monitor_service.py:108);
- an exception with no name now shows its ISO date (occupancy_service.py:259,303);
- the live label now uses the stored day_name (:429-431).
- Acceptance: the maintainer confirms these behaviors, or they are adjusted.

Refs #176, #113.

🤖 Generated with Claude Code

These P3 findings from the second review pass on #176 (business-day schedule records, #113) were deferred. None of them blocked the merge. ## Standards 1. **The audit action list is still written twice.** Pass 1 flagged this and it is only partly fixed. `ScheduleSource` now generates its DB CHECK, but the audit *action* values are still spelled out in two places: the Literal at `app/schemas/occupancy_models.py:390` and a hardcoded CHECK at `app/db/database.py:362`. `'FREEZE'` is also a bare string literal at `app/db/occupancy_repository.py:865`. - *Acceptance:* one definition of the audit actions, from which the CHECK is generated as for `ScheduleSource`, and the repository uses it instead of literals. 2. **Existing audit tables don't allow `FREEZE`.** `'FREEZE'` was added only to the `CREATE TABLE IF NOT EXISTS` CHECK (`database.py:362`). A database whose audit table was created by the pass-1 branch, such as the local validation copy of prod, rejects every automatic freeze with an IntegrityError inside the monitor. Prod is safe because the table is new there. - *Acceptance:* either a migration that rebuilds the CHECK when `FREEZE` is missing, or a note in the local-validation docs to recreate the database. 3. **The first-recorded schedule reports the wrong `recorded` flag.** `get_original_business_day_schedule_async` (`occupancy_repository.py:976`) decodes an audit `old_value` that can be an unrecorded fallback (`recorded: false`) through `BusinessDaySchedule.from_record`, which forces `recorded=True`. Only calibration reads it today. - *Acceptance:* the decoded value keeps its stored `recorded` flag, with a test. 4. **The CHECK constraints are built with f-strings** (`database.py:344-351`). There's no injection risk because the values come from a Literal, but others may copy the pattern. - *Acceptance:* build the constraint through one small helper that validates its input, or document why this is safe. 5. **Missing docstrings** on three of the four new service methods (`occupancy_service.py:192,205,209`). - *Acceptance:* each has a one-line docstring saying what it freezes or writes. ## Spec 6. **The default stamp stops at the last counted day.** Spec (#113): *"records can exist for any date, not only days with counted data"*. The default ends at the last counted event (`occupancy_service.py:357-360`), and the test asserts that `counted+3` stays unrecorded (test :168). A closed or outage stretch after counting stopped, up to yesterday, therefore stays on the fallback, and a later weekly edit would reclassify it. The same applies to days the monitor missed while it was down, because it freezes only the current day (`monitor_service.py:108-115`). - *Acceptance:* the default stamp covers every completed day up to `active_day - 1`, with the test updated. Alternatively, the maintainer decides to keep the counted span, and the help text says so. 7. **"First recorded value" depends on the order of audit rows.** Calibration reads a day's first audit row (`occupancy_repository.py:976-992`). A future IMPORTED writer that adds records without an audit row would make calibration follow whatever that record later becomes. An OVERWRITE never changes the calibration value, and that behavior is not documented. **This needs a design decision:** keep the audit-row invariant (documented in the docstring and enforced for every writer), or add an explicit `original_value` column. - *Acceptance:* the chosen rule is documented, and every record writer, including a future importer, follows it. 8. **Small behavior changes that were not asked for.** Record them in #113, or confirm them with the maintainer: - the monitor now re-reads the reset time only every 60 s or at the next boundary (`monitor_service.py:108`); - an exception with no name now shows its ISO date (`occupancy_service.py:259,303`); - the live label now uses the stored `day_name` (`:429-431`). - *Acceptance:* the maintainer confirms these behaviors, or they are adjusted. Refs #176, #113. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added this to the Historical passenger-flow backfill milestone

2026-09-29 15:45:45 +00:00

gabogg added the

needs-triage

label

2026-09-29 15:45:45 +00:00

gabogg referenced this issue

2026-09-29 15:45:46 +00:00

feat(schedule): each business day remembers the schedule it was measured under #176

gabogg commented

2026-10-02 15:42:53 +00:00

Author

Owner

Triage decision, 2026-10-02 (maintainer)

Item 6, the default stamp span: the default stamp covers every completed business day from the earliest day with any flow data (local or imported) up to active_day − 1, whether or not it has counted data. On startup the monitor backfills days it missed while down. Update the test (counted+3 must now be recorded).
- Before the historical import lands, that is the first local day.
- The import stamps its own days (see #170).
Item 7, the "first recorded value": store the day's Original Schedule explicitly.
- It is written once, when the day's schedule is first fixed, and corrections never change it.
- Calibration reads it, not the oldest audit row.
- This removes the dependence on audit-row order, so a future importer can't silently break it.
- An ADR records the decision, and CONTEXT.md gains the term (docs PR).
- For imported days, the Original Schedule is fixed at approval or publication. See the #170 note.
Item 8, behaviour changes:
1. The monitor re-reads the reset time every 60 s or at the next boundary: accepted.
2. A nameless exception showing its ISO date: replaced by a frontend-localized default ("Schedule exception" / "Excepción de horario"), per #184 / #73.
3. The live label uses the stored day_name: accepted.
Item 2, existing audit tables without FREEZE: add a generic migration that rebuilds the CHECK whenever the allowed action set differs from the single definition introduced by item 1. Any future action reuses it.
Items 1, 3, 4 and 5: as written.

Relabelled ready-for-agent.

🤖 Generated with Claude Code

## Triage decision, 2026-10-02 (maintainer) - **Item 6, the default stamp span:** the default stamp covers **every completed business day from the earliest day with any flow data (local or imported) up to `active_day − 1`**, whether or not it has counted data. On startup the monitor backfills days it missed while down. Update the test (`counted+3` must now be recorded). - Before the historical import lands, that is the first local day. - The import stamps its own days (see #170). - **Item 7, the "first recorded value":** store the day's **Original Schedule explicitly**. - It is written once, when the day's schedule is first fixed, and corrections never change it. - Calibration reads it, not the oldest audit row. - This removes the dependence on audit-row order, so a future importer can't silently break it. - An ADR records the decision, and CONTEXT.md gains the term (docs PR). - For imported days, the Original Schedule is fixed at approval or publication. See the #170 note. - **Item 8, behaviour changes:** 1. The monitor re-reads the reset time every 60 s or at the next boundary: accepted. 2. A nameless exception showing its ISO date: replaced by a frontend-localized default ("Schedule exception" / "Excepción de horario"), per #184 / #73. 3. The live label uses the stored `day_name`: accepted. - **Item 2, existing audit tables without `FREEZE`:** add a **generic migration** that rebuilds the CHECK whenever the allowed action set differs from the single definition introduced by item 1. Any future action reuses it. - **Items 1, 3, 4 and 5:** as written. Relabelled `ready-for-agent`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg referenced this issue

2026-10-02 15:42:54 +00:00

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

gabogg added

ready-for-agent

and removed

needs-triage

labels

2026-10-02 15:42:59 +00:00

gabogg referenced this issue from a commit

2026-10-02 15:47:47 +00:00

docs(domain): holidays are expected, not ordinary; store the Original Schedule