follow-up(statistics): P3 cleanups from PR #137 review #142

New issue

Closed

opened 2026-09-26 21:09:58 +00:00 by gabogg · 0 comments

gabogg commented

2026-09-26 21:09:58 +00:00

Owner

Summary of P3 Cleanups from PR #137 Review Pass 2

During the second code review pass for PR #137 (fix/cycle-verdict-ranking, addressing Issue #128), all functional and architectural requirements were verified and all 408 tests pass. The following minor cleanup opportunities (P3) were identified across standards and spec reviews:

1. Test helper naming clarity in `tests/test_statistics_baselines.py`

In test_statistics_baselines.py:245-247, an offset reset is seeded using add_verdict(reset_on_untrusted, "MANUAL_ADMIN", True) right below the comment stating that an offset reset is not a verdict:

# An offset reset is not a verdict: it cannot make an untrusted day usual (#128).
await seed_day(reset_on_untrusted, 10, trusted=False)
await add_verdict(reset_on_untrusted, "MANUAL_ADMIN", True)

Suggestion: Either rename add_verdict to seed_calibration_log or call record_calibration_log_async directly to avoid semantic dissonance between the comment and the helper name.

2. Test helper type precision in `tests/test_cycle_verdict.py`

In tests/test_cycle_verdict.py:128:

async def period_quality(granularity: str, day: date) -> PeriodQuality:

Suggestion: Annotate granularity with Literal["day", "week", "month"] or domain type rather than raw str.

3. SQL priority documentation for future calibration types

In app/db/occupancy_repository.py:64-76, _CYCLE_VERDICT_PRIORITY_SQL uses a 2-branch CASE WHEN (RETROACTIVE_GUARD_AUDIT -> 1, else 0).
Suggestion: If future audit types are added to _CYCLE_VERDICT_TYPES_SQL, ensure their priority ordering is explicitly defined rather than implicitly falling through to ELSE 0.

4. Stale comment cleanup in `app/services/analytics_service.py` (Deferred from #128)

As noted in Issue #128:
Line ~230 in analytics_service.py: 'oldest first, as the range query expects; the trust verdict is then read only until enough days pass'. Both halves are now obsolete as the range query accepts any order and trust evaluation is batched. The comment and any unnecessary sort can be cleaned up.

Maintainer triage — 2026-09-27

Proceed with concrete helper naming and granularity typing cleanups. Item 4 (stale oldest-first comment/sort) is already absent on current master: verify and mark complete rather than restoring or duplicating work. Document audit-priority requirements where helpful; introducing new audit types is outside scope.

### Summary of P3 Cleanups from PR #137 Review Pass 2 During the second code review pass for PR #137 (`fix/cycle-verdict-ranking`, addressing Issue #128), all functional and architectural requirements were verified and all 408 tests pass. The following minor cleanup opportunities (P3) were identified across standards and spec reviews: #### 1. Test helper naming clarity in `tests/test_statistics_baselines.py` In `test_statistics_baselines.py:245-247`, an offset reset is seeded using `add_verdict(reset_on_untrusted, "MANUAL_ADMIN", True)` right below the comment stating that an offset reset is not a verdict: ```python # An offset reset is not a verdict: it cannot make an untrusted day usual (#128). await seed_day(reset_on_untrusted, 10, trusted=False) await add_verdict(reset_on_untrusted, "MANUAL_ADMIN", True) ``` *Suggestion*: Either rename `add_verdict` to `seed_calibration_log` or call `record_calibration_log_async` directly to avoid semantic dissonance between the comment and the helper name. #### 2. Test helper type precision in `tests/test_cycle_verdict.py` In `tests/test_cycle_verdict.py:128`: ```python async def period_quality(granularity: str, day: date) -> PeriodQuality: ``` *Suggestion*: Annotate `granularity` with `Literal["day", "week", "month"]` or domain type rather than raw `str`. #### 3. SQL priority documentation for future calibration types In `app/db/occupancy_repository.py:64-76`, `_CYCLE_VERDICT_PRIORITY_SQL` uses a 2-branch `CASE WHEN` (`RETROACTIVE_GUARD_AUDIT` -> 1, else 0). *Suggestion*: If future audit types are added to `_CYCLE_VERDICT_TYPES_SQL`, ensure their priority ordering is explicitly defined rather than implicitly falling through to `ELSE 0`. #### 4. Stale comment cleanup in `app/services/analytics_service.py` (Deferred from #128) As noted in Issue #128: *Line ~230 in `analytics_service.py`*: `'oldest first, as the range query expects; the trust verdict is then read only until enough days pass'`. Both halves are now obsolete as the range query accepts any order and trust evaluation is batched. The comment and any unnecessary sort can be cleaned up. ## Maintainer triage — 2026-09-27 Proceed with concrete helper naming and granularity typing cleanups. Item 4 (stale oldest-first comment/sort) is already absent on current master: verify and mark complete rather than restoring or duplicating work. Document audit-priority requirements where helpful; introducing new audit types is outside scope.

gabogg added the

needs-triage

label

2026-09-26 21:09:58 +00:00

gabogg referenced this issue

2026-09-26 21:10:22 +00:00

fix(statistics): one ranked cycle verdict; offset resets are not verdicts #137

gabogg added

ready-for-agent

low-priority

and removed

needs-triage