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): saving the config form no longer resets calibration #115

Merged
gabogg merged 3 commits from fix/occupancy-config-save-preserves-calibration into master 2026-09-25 19:17:09 +00:00
Conversation 4 Commits 3 Files changed 5 +194 -47
gabogg commented 2026-09-25 18:29:57 +00:00
Owner
Copy link

Summary

Saving the admin occupancy config form reset the live exit multiplier (k) to 1.0 and overwrote the stored trust thresholds with defaults. Found while adding the #104 setting to the same form.

Problem

  • POST /api/occupancy/config validated the body with OccupancyConfigSchema and wrote payload.model_dump(): every field the form doesn't send (active_exit_multiplier, calibration_mode, multiplier_variance, …) arrived as its schema default and update_config_async wrote it. A calibrated k of 1.17 became 1.0 until the next calibration run; FLOW_RATE_DENSITY mode would revert to PROPORTIONAL_RATIO.
  • GET /api/occupancy/config built its response field by field and omitted the trust thresholds and calibration state, so the form's "Calibration and trust thresholds" inputs always showed defaults and saving re-wrote those defaults over stored values.
  • Reproduced on master: stored k = 1.17 and trust_ratio_min = 0.9 → GET shows 0.8 / 1.0 → form save → stored k = 1.0, trust_ratio_min = 0.8.

Production impact (corrected after review): every save of the config form reset k to 1.0, and also reset the seed initial_exit_multiplier (the form sends it, but GET never returned it, so it was always 1.0) and discarded any non-default trust threshold. The nightly auto-calibration does not rebuild k from history; it blends into the stored value (0.25 × empirical + 0.75 × stored), so a reset k kept pulling towards 1.0 for several nights, shrinking by 0.75 per trusted cycle, and the logged drift_value was skewed meanwhile. Paths that rebuild k from trusted history (refresh_active_multiplier_async: every app startup via anomaly reconciliation, trust toggles, retroactive audits; not the manual historical-reconciliation endpoint) restore it, but the rebuild seeds from initial_exit_multiplier, which the same bug reset. A save also cleared last_reset_date, which nothing reads.

Architectural impact

  • GET returns OccupancyConfigSchema.model_validate(stored_config).
  • POST takes a new OccupancyConfigUpdate body: every admin-editable setting with the same validation, minus the calibration-owned fields (CALIBRATION_OWNED_CONFIG_FIELDS: active exit multiplier, variance, mode, last-calibration markers), which are rejected with 422 VALIDATION_ERROR; unknown fields are rejected too.
  • OccupancyRepository.update_config_fields_async writes only the submitted fields in one UPDATE, so a calibration write between the admin's read and save is not lost.
  • Admin form (app.js): the baseline offset is sent only when the admin edited it (calibration also writes it overnight), and settings the form does not show (auto reset, auto scroll) are no longer sent.
  • No schema migration.

Verification

  • tests/test_occupancy_config_api.py: a form-shaped save (fields read from index.html) keeps k, variance, mode, the seed k, and an offset calibration wrote while the form was open, and saves the edited threshold; a single-field save changes only that field; each calibration-owned field is rejected. Fails on master.
  • pytest: 341 passed.
  • Review pass 2: the form now also shows the stored offset after a save (937141d), so a second save without reloading can't send a stale value, and never sends the offset if the config failed to load. Ruff, pre-commit and the docs check passed.

Checklist

  • GET shows stored values for every config field.
  • POST changes only submitted fields, in one statement; calibration-owned fields are rejected.
  • The form no longer overwrites the calibrated baseline offset or settings it does not show.
  • Regression test.

🤖 Generated with Claude Code

## Summary Saving the admin occupancy config form reset the **live exit multiplier (k) to 1.0** and overwrote the stored **trust thresholds** with defaults. Found while adding the #104 setting to the same form. ## Problem - `POST /api/occupancy/config` validated the body with `OccupancyConfigSchema` and wrote `payload.model_dump()`: every field the form doesn't send (`active_exit_multiplier`, `calibration_mode`, `multiplier_variance`, …) arrived as its schema default and `update_config_async` wrote it. A calibrated k of 1.17 became 1.0 until the next calibration run; FLOW_RATE_DENSITY mode would revert to PROPORTIONAL_RATIO. - `GET /api/occupancy/config` built its response field by field and omitted the trust thresholds and calibration state, so the form's "Calibration and trust thresholds" inputs always showed defaults and saving re-wrote those defaults over stored values. - Reproduced on `master`: stored k = 1.17 and `trust_ratio_min` = 0.9 → GET shows 0.8 / 1.0 → form save → stored k = 1.0, `trust_ratio_min` = 0.8. **Production impact (corrected after review):** every save of the config form reset k to 1.0, and also reset the seed `initial_exit_multiplier` (the form sends it, but GET never returned it, so it was always 1.0) and discarded any non-default trust threshold. The nightly auto-calibration does not rebuild k from history; it blends into the stored value (`0.25 × empirical + 0.75 × stored`), so a reset k kept pulling towards 1.0 for several nights, shrinking by 0.75 per trusted cycle, and the logged `drift_value` was skewed meanwhile. Paths that rebuild k from trusted history (`refresh_active_multiplier_async`: every **app startup** via anomaly reconciliation, trust toggles, retroactive audits; not the manual historical-reconciliation endpoint) restore it, but the rebuild seeds from `initial_exit_multiplier`, which the same bug reset. A save also cleared `last_reset_date`, which nothing reads. ## Architectural impact - GET returns `OccupancyConfigSchema.model_validate(stored_config)`. - POST takes a new `OccupancyConfigUpdate` body: every admin-editable setting with the same validation, minus the calibration-owned fields (`CALIBRATION_OWNED_CONFIG_FIELDS`: active exit multiplier, variance, mode, last-calibration markers), which are rejected with `422 VALIDATION_ERROR`; unknown fields are rejected too. - `OccupancyRepository.update_config_fields_async` writes only the submitted fields in one `UPDATE`, so a calibration write between the admin's read and save is not lost. - Admin form (`app.js`): the baseline offset is sent only when the admin edited it (calibration also writes it overnight), and settings the form does not show (auto reset, auto scroll) are no longer sent. - No schema migration. ## Verification - `tests/test_occupancy_config_api.py`: a form-shaped save (fields read from `index.html`) keeps k, variance, mode, the seed k, and an offset calibration wrote while the form was open, and saves the edited threshold; a single-field save changes only that field; each calibration-owned field is rejected. Fails on `master`. - `pytest`: 341 passed. - Review pass 2: the form now also shows the stored offset after a save (`937141d`), so a second save without reloading can't send a stale value, and never sends the offset if the config failed to load. Ruff, pre-commit and the docs check passed. ## Checklist - [x] GET shows stored values for every config field. - [x] POST changes only submitted fields, in one statement; calibration-owned fields are rejected. - [x] The form no longer overwrites the calibrated baseline offset or settings it does not show. - [x] Regression test. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
fix(occupancy): saving the config form no longer resets calibration
All checks were successful
CI / lint-and-test (pull_request) Successful in 1m40s
Details
0e91125959 
`POST /api/occupancy/config` wrote the full schema with defaults for every
field the admin form does not send, so saving the form reset the live exit
multiplier to 1.0 (and calibration mode / variance to their defaults) until
the next calibration run. `GET` also built its response by hand without the
trust thresholds, so the form showed defaults and re-saved them over the
stored values.

GET now returns the stored configuration, and POST merges only the fields
present in the request into the stored configuration.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-09-25 18:42:34 +00:00
Author
Owner
Copy link

Code review — pass 1 (two-axis)

Reviewed git diff origin/master...origin/fix/occupancy-config-save-preserves-calibration (0e91125). No originating issue: the Spec axis checked the fix against the bug report in the PR description. Standards and Spec ran independently and are reported separately. Severity: P1 must fix · P2 fix before merge · P3 minor. Per review policy, pass 1 findings are all fixed on the branch.

Standards

Ruff check/format pass; tests/test_occupancy_config_api.py passes. Branch name and commit message follow git-and-workflow.md; the test uses named constants.

Real bugs / risks

  • P2: daemon-owned fields can still be written (app/controllers/occupancy_controller.py:112–121). The docstring says calibration-daemon values "keep [their] stored value", but only when the client leaves them out. OccupancyConfigSchema still accepts active_exit_multiplier (no bounds), multiplier_variance, calibration_mode (any string, no enum) and last_calibrated_at / last_calibrated_offset. With exclude_unset, an admin can set just one of them (e.g. k=50) and it is merged and written. Already possible on master via the full dump. Suggest a separate OccupancyConfigUpdate request schema without these fields.
  • P3: lost update (read then write) (:120–121). The endpoint reads the whole row, merges, and writes the whole row back. If set_active_exit_multiplier_async or set_calibrated_offset_async commits between those awaits, stale values overwrite it. Narrow window, far better than master; a partial UPDATE in the repository would close it.
  • P3: GET now validates stored data (:107). model_validate checks stored initial_exit_multiplier against its 0.90–1.35 guardrails and the trust_* bounds. No path produces an out-of-range row (legacy seed 1.1162 is in range; trust_* are NOT NULL with in-range defaults; other writers go through the schema or typed setters). A hand-edited row would now 500 instead of showing defaults. Awareness only.
  • Exposure: none new. Newly returned fields were already in the old GET's response model (with defaults instead of stored values); stored k is already exposed to logged-in users by snapshot and statistics schemas.
  • Unmentioned side effect. Master's form save also wrote last_reset_date = ""; the merge now keeps the stored value. Nothing reads that column. Mention it in the description.

Documented-standard violations

  • Hard, P3, not introduced here. occupancy_controller.py:101, 110: both touched handlers lack a return annotation (AGENTS.md §2, code-standards.md §2: "explicit type hints … parameters and return types"). Add -> OccupancyConfigSchema.
  • Judgement, P3. Merge semantics live in the controller; AGENTS.md §1 says controllers are thin and delegate — this persistence logic belongs in the repository.

Baseline smells (judgement calls)

  • Possible Duplicated Code, P3. tests/test_occupancy_config_api.py:17–31: FORM_FIELDS copies the field list from saveOccupancyConfig in app.js; they can drift. Acceptable since the comment names the source.
  • Possible Primitive Obsession, P3 (schema not touched here). calibration_mode: str should be a Literal/enum, which also covers the P2.

Test gaps (P3)

No coverage of calibration_mode or multiplier_variance surviving a save (named as regressions in the PR), nor of a POST sending a single field.

Spec

Verdict: the fix is right for the stated problem; no P1. The "production impact" claim understates the damage.

Claims checked

  • Master reset these on every save: active_exit_multiplier, calibration_mode, multiplier_variance. The repo writes all three unconditionally (app/db/occupancy_repository.py:429-432) and master's model_dump() filled them with defaults. The trust thresholds use COALESCE, but the defaults are non-null, so they were overwritten too. Confirmed.
  • initial_exit_multiplier was also reset. The form sends it via data-config-key (index.html:773), but master's GET omitted it, so it was always saved as 1.0. The PR fixes this but doesn't mention it.
  • The regression test fails on master (throwaway worktree: assert 0.8 == 0.9) and passes on the branch; it covers both halves (GET-side assertion catches the omission, the final after[...] assertions catch the POST clobber).

(c) Overstated / understated claims

  • P2, understated impact (PR body). The PR says k was reset "until the calibration daemon's next run". The nightly path does not recompute k from history; it blends the stored k: new = 0.25*empirical_k + 0.75*old_multiplier (app/services/occupancy_service.py:~1150, via check_and_run_auto_calibration_async). A k reset to 1.0 carries into later nights, the error shrinking by 0.75 per trusted cycle; untrusted cycles keep 1.0. The logged drift_value is skewed too. Correct the impact statement, and note that a manual refresh_active_multiplier_async (analytics endpoint) restores k from trusted history.

(a) Not fixed / remaining clobbers

  • P2, app/static/js/app.js:2338. The form still posts baseline_offset loaded when the page opened. That field is also written by the calibration daemon (set_calibrated_offset_async, occupancy_repository.py:563) and the daily reset (:1636/1660). An admin tab left open across the quiet window overwrites that night's calibrated offset on the next save — the same kind of bug. Fix, or state out of scope and file an issue.
  • P3, app.js:2340-2342. The form hard-codes auto_reset_midnight: false and sends auto_scroll_* from hidden inputs loadOccupancyConfig never fills; both still overwrite stored values. No consumer today; low impact.
  • P3, occupancy_controller.py:120-121. Read-then-write without a transaction; a daemon write to k between the two is lost. Small window.

(b) Scope creep

None. GET moving to model_validate is needed for the fix.

Test gaps (P3)

  • tests/test_occupancy_config_api.py:59: the real form sends all 12 data-config-key fields; the test sends only trust_ratio_min, so it doesn't exactly match the UI payload.
  • No check that calibration_mode, multiplier_variance or initial_exit_multiplier are preserved, though the PR lists them.

Summary: Standards 10 (1×P2, 9×P3), worst: the request schema still lets a client set daemon-owned calibration fields (k, variance, mode). Spec 7 (2×P2, 5×P3), worst: the form still clobbers the nightly calibrated baseline_offset; also the PR understates impact (k reset persists through the 0.75 blend).

🤖 Generated with Claude Code

# Code review — pass 1 (two-axis) Reviewed `git diff origin/master...origin/fix/occupancy-config-save-preserves-calibration` (0e91125). No originating issue: the Spec axis checked the fix against the bug report in the PR description. Standards and Spec ran independently and are reported separately. Severity: **P1** must fix · **P2** fix before merge · **P3** minor. Per review policy, pass 1 findings are all fixed on the branch. ## Standards Ruff check/format pass; `tests/test_occupancy_config_api.py` passes. Branch name and commit message follow `git-and-workflow.md`; the test uses named constants. ### Real bugs / risks - **P2: daemon-owned fields can still be written** (`app/controllers/occupancy_controller.py:112–121`). The docstring says calibration-daemon values "keep [their] stored value", but only when the client leaves them out. `OccupancyConfigSchema` still accepts `active_exit_multiplier` (no bounds), `multiplier_variance`, `calibration_mode` (any string, no enum) and `last_calibrated_at` / `last_calibrated_offset`. With `exclude_unset`, an admin can set just one of them (e.g. k=50) and it is merged and written. Already possible on master via the full dump. Suggest a separate `OccupancyConfigUpdate` request schema without these fields. - **P3: lost update (read then write)** (`:120–121`). The endpoint reads the whole row, merges, and writes the whole row back. If `set_active_exit_multiplier_async` or `set_calibrated_offset_async` commits between those awaits, stale values overwrite it. Narrow window, far better than master; a partial `UPDATE` in the repository would close it. - **P3: GET now validates stored data** (`:107`). `model_validate` checks stored `initial_exit_multiplier` against its 0.90–1.35 guardrails and the `trust_*` bounds. No path produces an out-of-range row (legacy seed 1.1162 is in range; `trust_*` are `NOT NULL` with in-range defaults; other writers go through the schema or typed setters). A hand-edited row would now 500 instead of showing defaults. Awareness only. - **Exposure: none new.** Newly returned fields were already in the old GET's response model (with defaults instead of stored values); stored k is already exposed to logged-in users by snapshot and statistics schemas. - **Unmentioned side effect.** Master's form save also wrote `last_reset_date = ""`; the merge now keeps the stored value. Nothing reads that column. Mention it in the description. ### Documented-standard violations - **Hard, P3, not introduced here.** `occupancy_controller.py:101, 110`: both touched handlers lack a return annotation (AGENTS.md §2, code-standards.md §2: "explicit type hints … parameters and return types"). Add `-> OccupancyConfigSchema`. - **Judgement, P3.** Merge semantics live in the controller; AGENTS.md §1 says controllers are thin and delegate — this persistence logic belongs in the repository. ### Baseline smells (judgement calls) - **Possible Duplicated Code, P3.** `tests/test_occupancy_config_api.py:17–31`: `FORM_FIELDS` copies the field list from `saveOccupancyConfig` in `app.js`; they can drift. Acceptable since the comment names the source. - **Possible Primitive Obsession, P3 (schema not touched here).** `calibration_mode: str` should be a `Literal`/enum, which also covers the P2. ### Test gaps (P3) No coverage of `calibration_mode` or `multiplier_variance` surviving a save (named as regressions in the PR), nor of a POST sending a single field. ## Spec **Verdict:** the fix is right for the stated problem; no P1. The "production impact" claim **understates** the damage. ### Claims checked - **Master reset these on every save:** `active_exit_multiplier`, `calibration_mode`, `multiplier_variance`. The repo writes all three unconditionally (`app/db/occupancy_repository.py:429-432`) and master's `model_dump()` filled them with defaults. The trust thresholds use `COALESCE`, but the defaults are non-null, so they were overwritten too. Confirmed. - **`initial_exit_multiplier` was also reset.** The form sends it via `data-config-key` (`index.html:773`), but master's GET omitted it, so it was always saved as 1.0. The PR fixes this but doesn't mention it. - **The regression test fails on master** (throwaway worktree: `assert 0.8 == 0.9`) and passes on the branch; it covers both halves (GET-side assertion catches the omission, the final `after[...]` assertions catch the POST clobber). ### (c) Overstated / understated claims - **P2, understated impact (PR body).** The PR says k was reset "until the calibration daemon's next run". The nightly path does not recompute k from history; it blends the stored k: `new = 0.25*empirical_k + 0.75*old_multiplier` (`app/services/occupancy_service.py:~1150`, via `check_and_run_auto_calibration_async`). A k reset to 1.0 carries into later nights, the error shrinking by 0.75 per trusted cycle; untrusted cycles keep 1.0. The logged `drift_value` is skewed too. Correct the impact statement, and note that a manual `refresh_active_multiplier_async` (analytics endpoint) restores k from trusted history. ### (a) Not fixed / remaining clobbers - **P2, `app/static/js/app.js:2338`.** The form still posts `baseline_offset` loaded when the page opened. That field is also written by the calibration daemon (`set_calibrated_offset_async`, `occupancy_repository.py:563`) and the daily reset (`:1636/1660`). An admin tab left open across the quiet window overwrites that night's calibrated offset on the next save — the same kind of bug. Fix, or state out of scope and file an issue. - **P3, `app.js:2340-2342`.** The form hard-codes `auto_reset_midnight: false` and sends `auto_scroll_*` from hidden inputs `loadOccupancyConfig` never fills; both still overwrite stored values. No consumer today; low impact. - **P3, `occupancy_controller.py:120-121`.** Read-then-write without a transaction; a daemon write to k between the two is lost. Small window. ### (b) Scope creep None. GET moving to `model_validate` is needed for the fix. ### Test gaps (P3) - `tests/test_occupancy_config_api.py:59`: the real form sends all 12 `data-config-key` fields; the test sends only `trust_ratio_min`, so it doesn't exactly match the UI payload. - No check that `calibration_mode`, `multiplier_variance` or `initial_exit_multiplier` are preserved, though the PR lists them. --- **Summary:** Standards 10 (1×P2, 9×P3), worst: the request schema still lets a client set daemon-owned calibration fields (k, variance, mode). Spec 7 (2×P2, 5×P3), worst: the form still clobbers the nightly calibrated `baseline_offset`; also the PR understates impact (k reset persists through the 0.75 blend). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
fix(occupancy): admin config saves never touch calibration state
All checks were successful
CI / lint-and-test (pull_request) Successful in 1m45s
Details
01684ebcdd 
- `POST /api/occupancy/config` takes a new `OccupancyConfigUpdate` body:
  every admin-editable setting with the same validation, minus the
  calibration-owned fields (active exit multiplier, variance, mode,
  last-calibration markers), which are now rejected with VALIDATION_ERROR.
- The repository writes only the submitted fields in one UPDATE
  (`update_config_fields_async`), so a calibration write between reading
  and saving is no longer lost.
- The admin form sends the baseline offset only when the admin edited it,
  so a page left open overnight cannot overwrite the calibrated offset, and
  it no longer sends settings it does not show (auto reset, auto scroll).
- Handlers carry return annotations; tests follow the form's real payload
  (read from index.html), a single-field save, and rejection of each
  calibration-owned field.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-09-25 18:58:26 +00:00
Author
Owner
Copy link

Pass-1 findings addressed in 01684eb. Full suite 341 passed; ruff and docs check clean. The PR description is corrected (impact, design, verification).

Standards

Finding Fix
P2 daemon-owned fields still writable New OccupancyConfigUpdate request body (extra="forbid") = every admin-editable setting with the same validation, minus CALIBRATION_OWNED_CONFIG_FIELDS (k, variance, mode, last-calibration markers). Sending one returns 422 VALIDATION_ERROR; parametrized test per field.
P3 lost update (read then write) OccupancyRepository.update_config_fields_async writes only the submitted fields in one UPDATE (keys checked against OccupancyConfigUpdate), so a calibration write in between is no longer overwritten.
P3 GET validates stored data Awareness only; no change.
Unmentioned last_reset_date side effect Now in the PR description.
P3 missing return annotations -> OccupancyConfigSchema on both handlers.
P3 merge logic in the controller Moved to the repository (partial UPDATE).
P3 FORM_FIELDS duplicates app.js The numeric settings are now read from index.html's data-config-key inputs; the remaining named inputs stay listed with their source.
P3 calibration_mode: str No longer accepted from admins at all (calibration-owned). The response model keeps str — follow-up if a Literal is wanted there.
P3 test gaps Tests assert k, variance, mode and seed k survive a save, and a single-field save changes only that field.

Spec

Finding Fix
P2 understated impact Description rewritten: the nightly blend (0.25 × empirical + 0.75 × stored) carried the reset over several nights; paths that rebuild k from trusted history seed from initial_exit_multiplier, which the same bug reset.
initial_exit_multiplier reset unmentioned Now in the description and asserted by the test.
P2 form clobbers the calibrated baseline_offset Decision (maintainer): the form sends the offset only when the admin edited it (data-loaded tracking in app.js, refreshed after a save). Test: an offset written by calibration while the form is open survives the save.
P3 auto_reset_midnight / auto_scroll_* from unloaded inputs The form no longer sends settings it does not show; the API still accepts them.
P3 read-then-write without a transaction Same fix as the Standards lost-update item.
P3 test doesn't match the UI payload The test sends every data-config-key field from index.html plus the named inputs.

🤖 Generated with Claude Code

Pass-1 findings addressed in `01684eb`. Full suite 341 passed; ruff and docs check clean. The PR description is corrected (impact, design, verification). ### Standards | Finding | Fix | |---|---| | P2 daemon-owned fields still writable | New `OccupancyConfigUpdate` request body (`extra="forbid"`) = every admin-editable setting with the same validation, minus `CALIBRATION_OWNED_CONFIG_FIELDS` (k, variance, mode, last-calibration markers). Sending one returns `422 VALIDATION_ERROR`; parametrized test per field. | | P3 lost update (read then write) | `OccupancyRepository.update_config_fields_async` writes only the submitted fields in one `UPDATE` (keys checked against `OccupancyConfigUpdate`), so a calibration write in between is no longer overwritten. | | P3 GET validates stored data | Awareness only; no change. | | Unmentioned `last_reset_date` side effect | Now in the PR description. | | P3 missing return annotations | `-> OccupancyConfigSchema` on both handlers. | | P3 merge logic in the controller | Moved to the repository (partial `UPDATE`). | | P3 `FORM_FIELDS` duplicates app.js | The numeric settings are now read from `index.html`'s `data-config-key` inputs; the remaining named inputs stay listed with their source. | | P3 `calibration_mode: str` | No longer accepted from admins at all (calibration-owned). The response model keeps `str` — follow-up if a `Literal` is wanted there. | | P3 test gaps | Tests assert k, variance, mode and seed k survive a save, and a single-field save changes only that field. | ### Spec | Finding | Fix | |---|---| | P2 understated impact | Description rewritten: the nightly blend (`0.25 × empirical + 0.75 × stored`) carried the reset over several nights; paths that rebuild k from trusted history seed from `initial_exit_multiplier`, which the same bug reset. | | `initial_exit_multiplier` reset unmentioned | Now in the description and asserted by the test. | | P2 form clobbers the calibrated `baseline_offset` | Decision (maintainer): the form sends the offset only when the admin edited it (`data-loaded` tracking in `app.js`, refreshed after a save). Test: an offset written by calibration while the form is open survives the save. | | P3 `auto_reset_midnight` / `auto_scroll_*` from unloaded inputs | The form no longer sends settings it does not show; the API still accepts them. | | P3 read-then-write without a transaction | Same fix as the Standards lost-update item. | | P3 test doesn't match the UI payload | The test sends every `data-config-key` field from `index.html` plus the named inputs. | 🤖 Generated with [Claude Code](https://claude.com/claude-code)
fix(ui): keep the config form's baseline offset in step with the server
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m2s
Details
937141d639 
After a save the form tracked the stored offset as loaded but kept showing
the old value, so a second save without reloading sent the stale offset and
overwrote the one calibration had stored. The input now shows the stored
value too, and the offset is never sent when the config failed to load.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-09-25 19:14:01 +00:00
Author
Owner
Copy link

Code review — pass 2 (two-axis)

Re-reviewed at 01684eb. Each axis verified every pass-1 finding against the code, then reviewed the fix commit(s) for new problems. Verdicts: ✅ fixed · ◐ partial · ❌ not addressed · ↺ regressed · ⊘ declined with documented reason. Per policy, P1/P2 block merge; P3s go to one follow-up issue.

Standards

Pass-1 findings

Finding Verdict Evidence
P2 calibration-owned fields still writable ✅ fixed 01684eb app/schemas/occupancy_models.py:330–353: OccupancyConfigUpdate with extra="forbid" omits CALIBRATION_OWNED_CONFIG_FIELDS; POST uses it (occupancy_controller.py:113); parametrized 422 test per field (tests/test_occupancy_config_api.py:102–111). OpenAPI lists 25 fields, additionalProperties: false, guardrail bounds kept.
P3 lost update (read, write whole row) ✅ fixed Single partial UPDATE (occupancy_repository.py:414–432); test shows a calibration-written offset survives (:69–82).
P3 GET validates stored data ⊘ awareness only Never an action item.
last_reset_date side effect unmentioned ✅ fixed In the description; the partial UPDATE no longer writes it.
P3 missing return annotations ✅ fixed occupancy_controller.py:105, 116.
P3 merge logic in the controller ✅ fixed Controller is a one-line delegate.
P3 FORM_FIELDS duplicates app.js ◐ partial data-config-key fields read from index.html (:37–42); the 9 named inputs still copied by hand, source named. Acceptable.
P3 calibration_mode: str ⊘ declined with reason Admins can no longer send it; response model keeps str.
P3 test gaps ✅ fixed k, variance, mode, seed k and a single-field save covered.

New findings

  • P2: offset dirty-tracking goes stale after a save (app/static/js/app.js:2370). After a save only dataset.loaded was updated; the input kept the old value, so a second save without reloading sent it and overwrote the calibrated offset (refreshOccupancyData() doesn't reload the form). Fix: set both from the response.
  • P3: sends the offset if loading failed (app.js:2349): if the GET failed, dataset.loaded is undefined and "" !== undefined sends baseline_offset: 0. Guard with dataset.loaded !== undefined. (Clearing the input is a deliberate edit; non-admins get 403.)
  • P3: type checkers reject the dynamic schema (occupancy_models.py:344, annotations at occupancy_controller.py:113, occupancy_repository.py:420): create_model returns a variable, so pyright/mypy flag payload: OccupancyConfigUpdate and editors lose completion. No type checker in CI and OpenAPI renders correctly; a static subclass would fix it at the cost of duplication.
  • Checked OK: f-string SQL (occupancy_repository.py:423–427): column names only from keys Pydantic accepted with extra="forbid", re-checked against model_fields; values bound with ?. The ValueError guard has no test (P3).
  • Checked OK: bool→int: harmless but redundant (sqlite3 stores bools as 0/1); no nullable fields reach the UPDATE.
  • P3 possible Speculative Generality / dead code: update_config_async (occupancy_repository.py:434) has no caller left in app/; only tests seed with it.

ruff clean; pytest 340 passed, 1 skipped at 01684eb (throwaway worktree).

Merge readiness (this axis): Blocked by the P2 (stale offset after a save) — one-line fix. P3s → follow-up issue.

Spec

Pass-1 findings

Finding Verdict Evidence
P2 description understates impact (nightly blend) ✅ fixed The body now describes 0.25 × empirical + 0.75 × stored, matching occupancy_service.py:1149, and names the seed initial_exit_multiplier (:904). One precision gap remains (N2).
initial_exit_multiplier reset unmentioned ✅ fixed In the description; asserted at tests/test_occupancy_config_api.py:84.
P2 form clobbers the calibrated baseline_offset ◐ partial app.js:2033-2039, :2349-2352 send the offset only when edited (maintainer decision); the post-save bookkeeping reopens the hole (N1).
P3 auto_reset_midnight / auto_scroll_* from unloaded inputs ✅ fixed Removed from the payload (app.js:2335-2345); cfg-auto-scroll / cfg-scroll-interval are hidden and cfg-auto-reset doesn't exist, so no visible control loses effect.
P3 read-then-write without a transaction ✅ fixed One partial UPDATE, keys whitelisted against OccupancyConfigUpdate (occupancy_repository.py:414-432).
P3 test doesn't match the UI payload ✅ fixed Reads data-config-key inputs from index.html plus the named inputs (test_occupancy_config_api.py:24-44).
P3 mode, variance, seed k not asserted ✅ fixed test_occupancy_config_api.py:81-84, plus a parametrized 422 test per calibration-owned field.

Description claims checked: blend formula correct; last_reset_date correct (master cleared it; nothing in app/ reads it); flows that rebuild k partly correct (N2).

Other clients checked for new 422s: the only POST /config clients are app.js and tests; tests/test_occupancy.py:249,274,628 send only admin-editable fields; every write of calibration-owned fields goes through repository methods, not the endpoint; no CLI or script posts to /config. test_occupancy_config_api.py + test_occupancy.py: 25 passed (throwaway worktree).

New findings

  • P2 (N1), app/static/js/app.js:2370 (fix commit 01684eb): a second save still overwrites the calibrated offset. After a save, dataset.loaded is set to saved.baseline_offset, but the input keeps the old value and refreshOccupancyData() doesn't reload the form. Scenario: page loads offset 5 → calibration writes 8 overnight → admin saves (offset not sent, correct; loaded = "8", input still 5) → admin saves again → "5" !== "8", the form sends 5 and overwrites the calibrated value. Fix: write saved.baseline_offset into the input as well as loaded (also shows the admin the current value). Only the server side is tested.
  • P3 (N2), PR description: flows that rebuild k. refresh_active_multiplier_async is called by the trust toggle (analytics_controller.py:354), the retroactive audit (occupancy_service.py:1538) and reconcile_and_quarantine_historical_anomalies_async (:1468), which runs on every app startup (main.py:63). The manual historical-reconciliation endpoint (analytics_controller.py:331) does not. Replace "historical reconciliation" with "app startup" and note the rebuild still starts from the reset seed.

Merge readiness (this axis): Not ready until N1 (one-line JS change) is fixed; N2 goes with the description fix.

Summary: Standards: 1 P2 (form's offset baseline goes stale after a save) + 5 P3. Spec: the same P2 found independently + 1 P3 (flows that rebuild k named imprecisely in the description).

🤖 Generated with Claude Code

# Code review — pass 2 (two-axis) Re-reviewed at `01684eb`. Each axis verified every pass-1 finding against the code, then reviewed the fix commit(s) for new problems. Verdicts: ✅ fixed · ◐ partial · ❌ not addressed · ↺ regressed · ⊘ declined with documented reason. Per policy, P1/P2 block merge; P3s go to one follow-up issue. ## Standards ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P2 calibration-owned fields still writable | ✅ fixed | `01684eb` `app/schemas/occupancy_models.py:330–353`: `OccupancyConfigUpdate` with `extra="forbid"` omits `CALIBRATION_OWNED_CONFIG_FIELDS`; POST uses it (`occupancy_controller.py:113`); parametrized 422 test per field (`tests/test_occupancy_config_api.py:102–111`). OpenAPI lists 25 fields, `additionalProperties: false`, guardrail bounds kept. | | P3 lost update (read, write whole row) | ✅ fixed | Single partial `UPDATE` (`occupancy_repository.py:414–432`); test shows a calibration-written offset survives (`:69–82`). | | P3 GET validates stored data | ⊘ awareness only | Never an action item. | | `last_reset_date` side effect unmentioned | ✅ fixed | In the description; the partial UPDATE no longer writes it. | | P3 missing return annotations | ✅ fixed | `occupancy_controller.py:105, 116`. | | P3 merge logic in the controller | ✅ fixed | Controller is a one-line delegate. | | P3 `FORM_FIELDS` duplicates app.js | ◐ partial | `data-config-key` fields read from `index.html` (`:37–42`); the 9 named inputs still copied by hand, source named. Acceptable. | | P3 `calibration_mode: str` | ⊘ declined with reason | Admins can no longer send it; response model keeps `str`. | | P3 test gaps | ✅ fixed | k, variance, mode, seed k and a single-field save covered. | ### New findings - **P2: offset dirty-tracking goes stale after a save** (`app/static/js/app.js:2370`). After a save only `dataset.loaded` was updated; the input kept the old value, so a second save without reloading sent it and overwrote the calibrated offset (`refreshOccupancyData()` doesn't reload the form). Fix: set both from the response. - **P3: sends the offset if loading failed** (`app.js:2349`): if the GET failed, `dataset.loaded` is `undefined` and `"" !== undefined` sends `baseline_offset: 0`. Guard with `dataset.loaded !== undefined`. (Clearing the input is a deliberate edit; non-admins get 403.) - **P3: type checkers reject the dynamic schema** (`occupancy_models.py:344`, annotations at `occupancy_controller.py:113`, `occupancy_repository.py:420`): `create_model` returns a variable, so pyright/mypy flag `payload: OccupancyConfigUpdate` and editors lose completion. No type checker in CI and OpenAPI renders correctly; a static subclass would fix it at the cost of duplication. - **Checked OK: f-string SQL** (`occupancy_repository.py:423–427`): column names only from keys Pydantic accepted with `extra="forbid"`, re-checked against `model_fields`; values bound with `?`. The `ValueError` guard has no test (P3). - **Checked OK: bool→int**: harmless but redundant (sqlite3 stores bools as 0/1); no nullable fields reach the UPDATE. - **P3 possible Speculative Generality / dead code**: `update_config_async` (`occupancy_repository.py:434`) has no caller left in `app/`; only tests seed with it. ruff clean; pytest 340 passed, 1 skipped at `01684eb` (throwaway worktree). **Merge readiness (this axis):** Blocked by the P2 (stale offset after a save) — one-line fix. P3s → follow-up issue. ## Spec ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P2 description understates impact (nightly blend) | ✅ fixed | The body now describes `0.25 × empirical + 0.75 × stored`, matching `occupancy_service.py:1149`, and names the seed `initial_exit_multiplier` (`:904`). One precision gap remains (N2). | | `initial_exit_multiplier` reset unmentioned | ✅ fixed | In the description; asserted at `tests/test_occupancy_config_api.py:84`. | | P2 form clobbers the calibrated `baseline_offset` | ◐ partial | `app.js:2033-2039`, `:2349-2352` send the offset only when edited (maintainer decision); the post-save bookkeeping reopens the hole (N1). | | P3 `auto_reset_midnight` / `auto_scroll_*` from unloaded inputs | ✅ fixed | Removed from the payload (`app.js:2335-2345`); `cfg-auto-scroll` / `cfg-scroll-interval` are hidden and `cfg-auto-reset` doesn't exist, so no visible control loses effect. | | P3 read-then-write without a transaction | ✅ fixed | One partial `UPDATE`, keys whitelisted against `OccupancyConfigUpdate` (`occupancy_repository.py:414-432`). | | P3 test doesn't match the UI payload | ✅ fixed | Reads `data-config-key` inputs from `index.html` plus the named inputs (`test_occupancy_config_api.py:24-44`). | | P3 mode, variance, seed k not asserted | ✅ fixed | `test_occupancy_config_api.py:81-84`, plus a parametrized 422 test per calibration-owned field. | Description claims checked: blend formula correct; `last_reset_date` correct (master cleared it; nothing in `app/` reads it); flows that rebuild k partly correct (N2). Other clients checked for new 422s: the only `POST /config` clients are `app.js` and tests; `tests/test_occupancy.py:249,274,628` send only admin-editable fields; every write of calibration-owned fields goes through repository methods, not the endpoint; no CLI or script posts to `/config`. `test_occupancy_config_api.py` + `test_occupancy.py`: 25 passed (throwaway worktree). ### New findings - **P2 (N1), `app/static/js/app.js:2370` (fix commit 01684eb): a second save still overwrites the calibrated offset.** After a save, `dataset.loaded` is set to `saved.baseline_offset`, but the input keeps the old value and `refreshOccupancyData()` doesn't reload the form. Scenario: page loads offset 5 → calibration writes 8 overnight → admin saves (offset not sent, correct; `loaded = "8"`, input still `5`) → admin saves again → `"5" !== "8"`, the form sends 5 and overwrites the calibrated value. Fix: write `saved.baseline_offset` into the input as well as `loaded` (also shows the admin the current value). Only the server side is tested. - **P3 (N2), PR description: flows that rebuild k.** `refresh_active_multiplier_async` is called by the trust toggle (`analytics_controller.py:354`), the retroactive audit (`occupancy_service.py:1538`) and `reconcile_and_quarantine_historical_anomalies_async` (`:1468`), which runs on every **app startup** (`main.py:63`). The manual historical-reconciliation endpoint (`analytics_controller.py:331`) does not. Replace "historical reconciliation" with "app startup" and note the rebuild still starts from the reset seed. **Merge readiness (this axis):** Not ready until N1 (one-line JS change) is fixed; N2 goes with the description fix. --- **Summary:** Standards: 1 P2 (form's offset baseline goes stale after a save) + 5 P3. Spec: the same P2 found independently + 1 P3 (flows that rebuild k named imprecisely in the description). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
gabogg commented 2026-09-25 19:17:08 +00:00
Author
Owner
Copy link

Pass-2 blockers fixed; remaining P3s filed as #119. Combined master + #115 + #116: pytest 351 passed, 1 skipped; frontend 80/80. Merging.

🤖 Generated with Claude Code

Pass-2 blockers fixed; remaining P3s filed as #119. Combined master + #115 + #116: pytest 351 passed, 1 skipped; frontend 80/80. Merging. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
gabogg merged commit 8f4069f615 into master 2026-09-25 19:17:09 +00:00
gabogg deleted branch fix/occupancy-config-save-preserves-calibration 2026-09-25 19:17:09 +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!115
No description provided.