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

chore(statistics): review follow-up cleanups #169

Merged
gabogg merged 6 commits from chore/statistics-review-followups into master 2026-10-02 16:21:50 +00:00
Conversation 5 Commits 6 Files changed 7 +155 -14
gabogg commented 2026-09-28 00:15:39 +00:00
Owner
Copy link

Summary

Complete the scoped P3 follow-ups from #142 and #143. Rename the calibration-log test helper, type the cycle-verdict test helper's granularity, default the four PeriodQuality tier counters to zero, and enforce the existing SQL verdict-priority contract in code. The PR plan maps every issue item to a change or a verified accepted no-op.

Architectural impact

Production changes are zero defaults for the four PeriodQuality tier counters in app/schemas/statistics.py and the cycle-verdict priority contract enforcement in app/db/occupancy_repository.py:96-112, where _CYCLE_VERDICT_PRIORITIES generates both _CYCLE_VERDICT_TYPES_SQL and _CYCLE_VERDICT_PRIORITY_SQL. Existing API fields and runtime cycle-verdict ordering remain intact. DayQualityState stays in its single service owner, and no shared test fixture or new audit type is introduced.

Verification / test evidence

  • python3 scripts/check_docs.py: 39 Markdown files and 80 HTTP operations checked.
  • .venv/bin/ruff check .: passed.
  • .venv/bin/ruff format --check .: passed (150 files).
  • pytest -q: 521 passed.
  • Commit hook: whitespace, file endings, Ruff, formatting, and pytest passed.

Checklist

  • Map every #142/#143 item to a change or accepted no-op in docs/audit/statistics-review-followups-pr-plan.md.
  • Complete scoped implementation and regression tests.
  • Push implementation commit c3a06b9.
  • Push pass 1 review fixes commit 3a728b7.
  • Push pass 2 review fixes commit 2744522.
  • Complete required review passes and address findings before merge.

Closes #142
Closes #143

Review protocol

This PR addresses follow-up issues, so the repository's three-pass review path applies. First pass: address every P1/P2/P3 finding. Second pass: address every P1/P2/P3 finding and request a third pass. Third pass: address P1/P2 findings and file linked follow-up issues for deferred P3 findings before merge.

## Summary Complete the scoped P3 follow-ups from #142 and #143. Rename the calibration-log test helper, type the cycle-verdict test helper's granularity, default the four `PeriodQuality` tier counters to zero, and enforce the existing SQL verdict-priority contract in code. The PR plan maps every issue item to a change or a verified accepted no-op. ## Architectural impact Production changes are zero defaults for the four `PeriodQuality` tier counters in `app/schemas/statistics.py` and the cycle-verdict priority contract enforcement in `app/db/occupancy_repository.py:96-112`, where `_CYCLE_VERDICT_PRIORITIES` generates both `_CYCLE_VERDICT_TYPES_SQL` and `_CYCLE_VERDICT_PRIORITY_SQL`. Existing API fields and runtime cycle-verdict ordering remain intact. `DayQualityState` stays in its single service owner, and no shared test fixture or new audit type is introduced. ## Verification / test evidence - `python3 scripts/check_docs.py`: 39 Markdown files and 80 HTTP operations checked. - `.venv/bin/ruff check .`: passed. - `.venv/bin/ruff format --check .`: passed (150 files). - `pytest -q`: 521 passed. - Commit hook: whitespace, file endings, Ruff, formatting, and pytest passed. ## Checklist - [x] Map every #142/#143 item to a change or accepted no-op in `docs/audit/statistics-review-followups-pr-plan.md`. - [x] Complete scoped implementation and regression tests. - [x] Push implementation commit `c3a06b9`. - [x] Push pass 1 review fixes commit `3a728b7`. - [x] Push pass 2 review fixes commit `2744522`. - [ ] Complete required review passes and address findings before merge. Closes #142 Closes #143 ## Review protocol This PR addresses follow-up issues, so the repository's three-pass review path applies. First pass: address every P1/P2/P3 finding. Second pass: address every P1/P2/P3 finding and request a third pass. Third pass: address P1/P2 findings and file linked follow-up issues for deferred P3 findings before merge.
docs(statistics): map review follow-up work
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m31s
Details
bebb8a47ac
Merge remote-tracking branch 'origin/master' into chore/statistics-review-followups
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m12s
Details
295beb20b4
chore(statistics): complete review follow-up cleanups
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m11s
Details
c3a06b9bfe
gabogg changed title from WIP: chore(statistics): review follow-up cleanups to chore(statistics): review follow-up cleanups 2026-09-29 23:50:22 +00:00
Merge remote-tracking branch 'origin/master' into chore/statistics-review-followups
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m40s
Details
14f03243f4
gabogg reviewed 2026-10-02 14:40:28 +00:00
gabogg left a comment
Copy link

Code review, pass 1 (origin/master...14f0324, spec #142, #143)

Result: no P1s or P2s, and 10 P3s. The spec is met. This PR addresses follow-up issues, so it takes the three-pass path: every finding on passes 1 and 2 gets fixed.

Verification:

  • ruff: clean.
  • tests/test_cycle_verdict.py, test_statistics_baselines.py and test_statistics_marker_inputs.py: 33 passed.
  • Every PeriodQuality/SelectablePeriod constructor and every frontend reader of the tier counters was grepped.

Standards

The code has no documented-standard violations. SQL stays in the repository, types are annotated, and the tests use a real sqlite DB with an ASGI client.

The zero defaults don't hide missing data today.

  • The only production constructor, analytics_service.py:840, passes all four counters.
  • SelectablePeriod (:881) spreads quality.model_dump().
  • The frontend already treats a missing value as falsy.

P3

  1. The defaults pick the optimistic answer (app/schemas/statistics.py:38-41).
    • missing_days/unreliable_days = 0 reads as "clean", so a future constructor that omits them silently shows a fully trusted period. The fields also become optional in OpenAPI.
    • Either keep them required, or add a comment that producers must always set them.
  2. The API test name overstates what it proves (tests/test_cycle_verdict.py:~292-355). test_period_quality_tier_counters_serialized_in_api_responses never reaches the defaults: the service computes real zeros. It is a key-set contract test.
  3. Duplicated Code in the tests. The same five counter assertions appear 4 times, and the 16-key list appears twice. Use one TIER_COUNTERS/QUALITY_KEYS constant.
  4. The priority rule is a comment, not code (judgement call, occupancy_repository.py:95-100). The new comment documents the priority contract instead of enforcing it. A type→priority dict generating both _CYCLE_VERDICT_TYPES_SQL and the CASE would make it impossible to forget one. The comment is acceptable as the minimal #142 fix.
  5. Misleading name (tests/test_cycle_verdict.py:~271). today = yesterday() should be day.
  6. docs/audit/statistics-review-followups-pr-plan.md is not linked from docs/README.md (55-58).
  7. The plan doc holds agent-process notes that will go stale: the "worker could not run rtk ruff check ." line and the Python 3.14 teardown note. These belong in the PR body, if anywhere. Also confirm the "maintainer triage decision" wording against #143.

Spec

Checklist:

  • #142:
    • done: the add_verdict → seed_calibration_log rename (no leftovers), Granularity typing, and the SQL priority contract comment;
    • verified as a no-op: the stale oldest-first comment.
  • #143:
    • done: the tier-counter defaults, with the partial-construction and API tests;
    • no-op per triage: DayQualityState (no second consumer);
    • not extracted, per triage: the shared fixture.

P3

  • A. The twin helper is still typed str. tests/test_statistics_marker_inputs.py:76 period_quality(granularity: str, ...). #143.3 asks for "typed Granularity literals", and triage made only the extraction conditional.
  • B. The plan doc's verification evidence conflicts with the PR description (plan:30). It says uvx ruff check ., while the PR says .venv/bin/ruff check ..
  • C. The plan doc still calls itself a "draft PR plan" (line 3: "This draft will close both...").

Scope creep: none in the code. The new API-response test is justified by triage's "verifying existing response… behavior".

Summary

  • Standards: 7 P3s. The worst is the optimistic zero defaults.
  • Spec: 3 P3s. The worst is the twin helper left typed str.

🤖 Generated with Claude Code

## Code review, pass 1 (`origin/master...14f0324`, spec #142, #143) Result: **no P1s or P2s, and 10 P3s.** The spec is met. This PR addresses follow-up issues, so it takes the three-pass path: every finding on passes 1 and 2 gets fixed. Verification: - ruff: clean. - `tests/test_cycle_verdict.py`, `test_statistics_baselines.py` and `test_statistics_marker_inputs.py`: 33 passed. - Every `PeriodQuality`/`SelectablePeriod` constructor and every frontend reader of the tier counters was grepped. ## Standards The code has no documented-standard violations. SQL stays in the repository, types are annotated, and the tests use a real sqlite DB with an ASGI client. **The zero defaults don't hide missing data today.** - The only production constructor, `analytics_service.py:840`, passes all four counters. - `SelectablePeriod` (:881) spreads `quality.model_dump()`. - The frontend already treats a missing value as falsy. **P3** 1. **The defaults pick the optimistic answer** (`app/schemas/statistics.py:38-41`). - `missing_days`/`unreliable_days = 0` reads as "clean", so a future constructor that omits them silently shows a fully trusted period. The fields also become optional in OpenAPI. - Either keep them required, or add a comment that producers must always set them. 2. **The API test name overstates what it proves** (`tests/test_cycle_verdict.py:~292-355`). `test_period_quality_tier_counters_serialized_in_api_responses` never reaches the defaults: the service computes real zeros. It is a key-set contract test. 3. **Duplicated Code in the tests.** The same five counter assertions appear 4 times, and the 16-key list appears twice. Use one `TIER_COUNTERS`/`QUALITY_KEYS` constant. 4. **The priority rule is a comment, not code** (judgement call, `occupancy_repository.py:95-100`). The new comment documents the priority contract instead of enforcing it. A type→priority dict generating both `_CYCLE_VERDICT_TYPES_SQL` and the CASE would make it impossible to forget one. The comment is acceptable as the minimal #142 fix. 5. **Misleading name** (`tests/test_cycle_verdict.py:~271`). `today = yesterday()` should be `day`. 6. **`docs/audit/statistics-review-followups-pr-plan.md`** is not linked from `docs/README.md` (55-58). 7. **The plan doc holds agent-process notes that will go stale**: the "worker could not run `rtk ruff check .`" line and the Python 3.14 teardown note. These belong in the PR body, if anywhere. Also confirm the "maintainer triage decision" wording against #143. ## Spec **Checklist:** - **#142:** - done: the `add_verdict` → `seed_calibration_log` rename (no leftovers), `Granularity` typing, and the SQL priority contract comment; - verified as a no-op: the stale oldest-first comment. - **#143:** - done: the tier-counter defaults, with the partial-construction and API tests; - no-op per triage: `DayQualityState` (no second consumer); - not extracted, per triage: the shared fixture. **P3** - **A. The twin helper is still typed `str`.** `tests/test_statistics_marker_inputs.py:76` `period_quality(granularity: str, ...)`. #143.3 asks for *"typed Granularity literals"*, and triage made only the extraction conditional. - **B. The plan doc's verification evidence conflicts with the PR description** (`plan:30`). It says `uvx ruff check .`, while the PR says `.venv/bin/ruff check .`. - **C. The plan doc still calls itself a "draft PR plan"** (line 3: "This draft will close both..."). **Scope creep:** none in the code. The new API-response test is justified by triage's "verifying existing response… behavior". ## Summary - **Standards:** 7 P3s. The worst is the optimistic zero defaults. - **Spec:** 3 P3s. The worst is the twin helper left typed `str`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
fix(review): address first review of PR 169
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m40s
Details
3a728b763e
gabogg commented 2026-10-02 14:55:48 +00:00
Author
Owner
Copy link

Pass 1 fixes (3a728b7)

All 10 findings from review pass 1 have been addressed in 3a728b7:

Standards

  1. Defaults pick optimistic answer (app/schemas/statistics.py): Added an explicit comment on PeriodQuality stating that producers (analytics_service) must always populate all tier counters explicitly rather than relying on defaults to assume clean data.
  2. API test name overstates what it proves (tests/test_cycle_verdict.py): Renamed test to test_period_quality_tier_counter_keys_serialized_in_api_responses and documented it as a key-set contract test for period endpoints.
  3. Duplicated code in tests (tests/test_cycle_verdict.py): Defined reusable TIER_COUNTERS, QUALITY_KEYS, and SELECTABLE_PERIOD_KEYS constants, replacing duplicated 5-line counter assertions and repeated 16-key lists.
  4. Priority rule is comment, not code (app/db/occupancy_repository.py): Created _CYCLE_VERDICT_PRIORITIES: dict[str, int] mapping eligible verdict types to priority ranks, generating both _CYCLE_VERDICT_TYPES_SQL and _CYCLE_VERDICT_PRIORITY_SQL from the dictionary.
  5. Misleading name (tests/test_cycle_verdict.py): Renamed today = yesterday() to day = yesterday().
  6. Plan doc not linked from docs/README.md: Added [Statistics review follow-ups plan](audit/statistics-review-followups-pr-plan.md) under Audit snapshots in docs/README.md.
  7. Stale agent-process notes and triage wording in plan doc: Stripped local execution and Python 3.14 teardown process notes from docs/audit/statistics-review-followups-pr-plan.md, and aligned maintainer triage wording with #143.

Spec

  • A. Twin helper still typed str (tests/test_statistics_marker_inputs.py): Imported Granularity and typed granularity: Granularity in period_quality(granularity: Granularity, start: date).
  • B. Verification command discrepancy in plan doc: Updated verification evidence in the plan doc to .venv/bin/ruff check . to match the PR description.
  • C. Plan doc titled "draft PR plan": Removed "draft" from title and body of docs/audit/statistics-review-followups-pr-plan.md.

Ready for review pass 2.

## Pass 1 fixes (3a728b7) All 10 findings from review pass 1 have been addressed in `3a728b7`: ### Standards 1. **Defaults pick optimistic answer (`app/schemas/statistics.py`)**: Added an explicit comment on `PeriodQuality` stating that producers (`analytics_service`) must always populate all tier counters explicitly rather than relying on defaults to assume clean data. 2. **API test name overstates what it proves (`tests/test_cycle_verdict.py`)**: Renamed test to `test_period_quality_tier_counter_keys_serialized_in_api_responses` and documented it as a key-set contract test for period endpoints. 3. **Duplicated code in tests (`tests/test_cycle_verdict.py`)**: Defined reusable `TIER_COUNTERS`, `QUALITY_KEYS`, and `SELECTABLE_PERIOD_KEYS` constants, replacing duplicated 5-line counter assertions and repeated 16-key lists. 4. **Priority rule is comment, not code (`app/db/occupancy_repository.py`)**: Created `_CYCLE_VERDICT_PRIORITIES: dict[str, int]` mapping eligible verdict types to priority ranks, generating both `_CYCLE_VERDICT_TYPES_SQL` and `_CYCLE_VERDICT_PRIORITY_SQL` from the dictionary. 5. **Misleading name (`tests/test_cycle_verdict.py`)**: Renamed `today = yesterday()` to `day = yesterday()`. 6. **Plan doc not linked from `docs/README.md`**: Added `[Statistics review follow-ups plan](audit/statistics-review-followups-pr-plan.md)` under Audit snapshots in `docs/README.md`. 7. **Stale agent-process notes and triage wording in plan doc**: Stripped local execution and Python 3.14 teardown process notes from `docs/audit/statistics-review-followups-pr-plan.md`, and aligned maintainer triage wording with #143. ### Spec - **A. Twin helper still typed `str` (`tests/test_statistics_marker_inputs.py`)**: Imported `Granularity` and typed `granularity: Granularity` in `period_quality(granularity: Granularity, start: date)`. - **B. Verification command discrepancy in plan doc**: Updated verification evidence in the plan doc to `.venv/bin/ruff check .` to match the PR description. - **C. Plan doc titled "draft PR plan"**: Removed "draft" from title and body of `docs/audit/statistics-review-followups-pr-plan.md`. Ready for review pass 2.
gabogg reviewed 2026-10-02 15:06:50 +00:00
gabogg left a comment
Copy link

Code review, pass 2 (origin/master...3a728b7, spec #142, #143)

Result: no P1s, no P2s and 5 P3s. The plan-doc finding appears on both axes; the Standards and Spec copies are one issue. This PR addresses follow-up issues, so it takes the three-pass path: fix every P3 below, push, and request a third pass.

Verification:

  • ruff check and format: clean.
  • check_docs.py: 0 errors.
  • tests/test_cycle_verdict.py, test_statistics_marker_inputs.py and test_statistics_baselines.py: 33 passed.
  • The generated verdict SQL matches master's: the same IN set (order only differs), and the same CASE ranking (RETROACTIVE_GUARD_AUDIT = 1, everything else 0). Only occupancy_repository.py:1703,1706 uses it.

Standards

Pass-1 fixes: all 7 are fixed, with no regressions.

  • The defaults comment is in place.
  • The test is renamed and has a docstring.
  • The TIER_COUNTERS/QUALITY_KEYS/SELECTABLE_PERIOD_KEYS constants replace the repeated assertions.
  • _CYCLE_VERDICT_PRIORITIES now generates both SQL fragments.
  • today is now day.
  • The README link is in.
  • The process notes are gone.

P3

  1. The plan doc misdescribes the #142 item 3 fix (docs/audit/statistics-review-followups-pr-plan.md:22, plus line 7 in Scope). It says "Documented the cycle-verdict priority contract…", but fix 4 replaced that comment with _CYCLE_VERDICT_PRIORITIES, which generates both fragments. Say the contract is enforced by the dict.
  2. Two quoting styles in one hunk (judgement call, occupancy_repository.py:104 vs :109). The IN list quotes with repr(k), the CASE with f"'{k}'". For a key containing ', repr switches to double quotes, which SQLite reads as an identifier. Use f"'{k}'" in both.
  3. Mysterious Name (judgement call, tests/test_cycle_verdict.py:249).
    • TIER_COUNTERS = {"closed_days": 0, …} is a dict of expected zeros, not a list of counters.
    • It includes closed_days, which isn't one of the tiers the schema comment names.
    • Rename it to something like ZERO_DAY_COUNTERS.

Spec

Pass-1 fix claims (c3400): all true.

  • Standards 1–7 are fixed.
  • A: the twin helper is now typed Granularity (test_statistics_marker_inputs.py:76).
  • B: the plan doc now says .venv/bin/ruff check ..
  • C: "draft" is gone.
  • The triage wording now matches #143: "only when an actual second consumer justifies it".

#142 / #143 checklist:

  • #142.1, the rename: done.
  • #142.2, granularity typing: done.
  • #142.3, verdict priority: done, now enforced in code.
  • #142.4: a no-op, per triage.
  • #143.1, the counter defaults: done, with tests.
  • #143.2 and #143.3: no-ops, per triage. Both helpers are now typed.

P3

  • A. The plan doc still describes a comment for #142 item 3 (same as Standards P3-1).
  • B. The PR description is stale.
    • It still says "document the existing SQL verdict-priority contract".
    • It doesn't mention the repository change at occupancy_repository.py:96-112.
    • Its checklist lists only c3a06b9, not 3a728b7.
    • It cites the old docs-check count (37 files / 72 operations); the check now reports 39 / 80.
  • C. Scope (accepted). The priority dict goes beyond #142's triage, "Document audit-priority requirements where helpful; introducing new audit types is outside scope." It adds no audit type and keeps behaviour, so it's accepted; just disclose it in the plan doc and PR body (see A and B).

Summary

  • Standards: 3 P3s. The worst is the plan doc misdescribing the #142 item 3 fix.
  • Spec: 3 P3s, one shared with Standards. The worst is the stale PR description.

🤖 Generated with Claude Code

## Code review, pass 2 (`origin/master...3a728b7`, spec #142, #143) Result: **no P1s, no P2s and 5 P3s.** The plan-doc finding appears on both axes; the Standards and Spec copies are one issue. This PR addresses follow-up issues, so it takes the three-pass path: fix every P3 below, push, and request a **third pass**. Verification: - ruff check and format: clean. - `check_docs.py`: 0 errors. - `tests/test_cycle_verdict.py`, `test_statistics_marker_inputs.py` and `test_statistics_baselines.py`: 33 passed. - The generated verdict SQL matches master's: the same `IN` set (order only differs), and the same CASE ranking (RETROACTIVE_GUARD_AUDIT = 1, everything else 0). Only `occupancy_repository.py:1703,1706` uses it. ## Standards **Pass-1 fixes:** all 7 are fixed, with no regressions. - The defaults comment is in place. - The test is renamed and has a docstring. - The `TIER_COUNTERS`/`QUALITY_KEYS`/`SELECTABLE_PERIOD_KEYS` constants replace the repeated assertions. - `_CYCLE_VERDICT_PRIORITIES` now generates both SQL fragments. - `today` is now `day`. - The README link is in. - The process notes are gone. **P3** 1. **The plan doc misdescribes the #142 item 3 fix** (`docs/audit/statistics-review-followups-pr-plan.md:22`, plus line 7 in Scope). It says "Documented the cycle-verdict priority contract…", but fix 4 replaced that comment with `_CYCLE_VERDICT_PRIORITIES`, which generates both fragments. Say the contract is enforced by the dict. 2. **Two quoting styles in one hunk** (judgement call, `occupancy_repository.py:104` vs `:109`). The `IN` list quotes with `repr(k)`, the CASE with `f"'{k}'"`. For a key containing `'`, `repr` switches to double quotes, which SQLite reads as an identifier. Use `f"'{k}'"` in both. 3. **Mysterious Name** (judgement call, `tests/test_cycle_verdict.py:249`). - `TIER_COUNTERS = {"closed_days": 0, …}` is a dict of expected zeros, not a list of counters. - It includes `closed_days`, which isn't one of the tiers the schema comment names. - Rename it to something like `ZERO_DAY_COUNTERS`. ## Spec **Pass-1 fix claims (c3400):** all true. - Standards 1–7 are fixed. - A: the twin helper is now typed `Granularity` (`test_statistics_marker_inputs.py:76`). - B: the plan doc now says `.venv/bin/ruff check .`. - C: "draft" is gone. - The triage wording now matches #143: *"only when an actual second consumer justifies it"*. **#142 / #143 checklist:** - #142.1, the rename: done. - #142.2, granularity typing: done. - #142.3, verdict priority: done, now enforced in code. - #142.4: a no-op, per triage. - #143.1, the counter defaults: done, with tests. - #143.2 and #143.3: no-ops, per triage. Both helpers are now typed. **P3** - **A. The plan doc still describes a comment for #142 item 3** (same as Standards P3-1). - **B. The PR description is stale.** - It still says "document the existing SQL verdict-priority contract". - It doesn't mention the repository change at `occupancy_repository.py:96-112`. - Its checklist lists only c3a06b9, not 3a728b7. - It cites the old docs-check count (37 files / 72 operations); the check now reports 39 / 80. - **C. Scope** (accepted). The priority dict goes beyond #142's triage, *"Document audit-priority requirements where helpful; introducing new audit types is outside scope."* It adds no audit type and keeps behaviour, so it's accepted; just disclose it in the plan doc and PR body (see A and B). ## Summary - **Standards:** 3 P3s. The worst is the plan doc misdescribing the #142 item 3 fix. - **Spec:** 3 P3s, one shared with Standards. The worst is the stale PR description. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
fix(review): address second review of PR 169
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m42s
Details
2744522ddf
gabogg commented 2026-10-02 16:10:48 +00:00
Author
Owner
Copy link

Pass 2 fixes (2744522)

All 5 P3 findings from review pass 2 have been addressed in 2744522:

Standards

  1. Plan doc misdescribes #142 item 3 fix (docs/audit/statistics-review-followups-pr-plan.md): Updated Scope line 7 and resolution checklist item 3 to state that the priority contract is enforced by _CYCLE_VERDICT_PRIORITIES generating both SQL fragments, replacing comment-only wording.
  2. Two quoting styles in one hunk (app/db/occupancy_repository.py): Formatted _CYCLE_VERDICT_TYPES_SQL using f"'{k}'", unifying single-quote formatting across both _CYCLE_VERDICT_TYPES_SQL and _CYCLE_VERDICT_PRIORITY_SQL.
  3. Mysterious Name (tests/test_cycle_verdict.py): Renamed TIER_COUNTERS to ZERO_DAY_COUNTERS across definition and all assertion sites, clarifying that it represents expected zero counts (including closed_days).

Spec

  • A. Plan doc describes comment for #142 item 3: Resolved in conjunction with Standards 1 by describing dictionary-based code enforcement in the plan doc.
  • B. Stale PR description: Updated the PR description via tea pr edit 169 to:
    • Note code enforcement of the verdict-priority contract in the summary.
    • Detail occupancy_repository.py:96-112 in architectural impact.
    • Record pass 1 (3a728b7) and pass 2 (2744522) fix commits in the checklist.
    • Update doc check evidence to current counts (39 files / 80 operations).
  • C. Priority dict scope disclosure: Disclosed dictionary enforcement in both the plan doc and the PR description, noting existing behaviour is preserved without adding new audit types.

Ready for review pass 3.

## Pass 2 fixes (2744522) All 5 P3 findings from review pass 2 have been addressed in `2744522`: ### Standards 1. **Plan doc misdescribes #142 item 3 fix (`docs/audit/statistics-review-followups-pr-plan.md`)**: Updated Scope line 7 and resolution checklist item 3 to state that the priority contract is enforced by `_CYCLE_VERDICT_PRIORITIES` generating both SQL fragments, replacing comment-only wording. 2. **Two quoting styles in one hunk (`app/db/occupancy_repository.py`)**: Formatted `_CYCLE_VERDICT_TYPES_SQL` using `f"'{k}'"`, unifying single-quote formatting across both `_CYCLE_VERDICT_TYPES_SQL` and `_CYCLE_VERDICT_PRIORITY_SQL`. 3. **Mysterious Name (`tests/test_cycle_verdict.py`)**: Renamed `TIER_COUNTERS` to `ZERO_DAY_COUNTERS` across definition and all assertion sites, clarifying that it represents expected zero counts (including `closed_days`). ### Spec - **A. Plan doc describes comment for #142 item 3**: Resolved in conjunction with Standards 1 by describing dictionary-based code enforcement in the plan doc. - **B. Stale PR description**: Updated the PR description via `tea pr edit 169` to: - Note code enforcement of the verdict-priority contract in the summary. - Detail `occupancy_repository.py:96-112` in architectural impact. - Record pass 1 (`3a728b7`) and pass 2 (`2744522`) fix commits in the checklist. - Update doc check evidence to current counts (39 files / 80 operations). - **C. Priority dict scope disclosure**: Disclosed dictionary enforcement in both the plan doc and the PR description, noting existing behaviour is preserved without adding new audit types. Ready for review pass 3.
gabogg reviewed 2026-10-02 16:21:34 +00:00
gabogg left a comment
Copy link

Code review, pass 3 (origin/master...2744522, spec #142, #143)

Result: no P1s, no P2s and no P3s. This is the third pass of a follow-up PR, with nothing to fix and nothing to defer, so it merges.

Verification:

  • ruff check and format: clean.
  • check_docs.py: 39 Markdown files and 80 HTTP operations checked, 0 errors.
  • tests/test_cycle_verdict.py, test_statistics_marker_inputs.py and test_statistics_baselines.py: 33 passed.
  • Wider set (test_cycle_verdict, test_calibration_*, test_occupancy*, test_statistics_*, test_trust_dataset_parity): 152 passed.
  • A SQL probe printed both generated fragments and compared them with master:
    • IN ('RETROACTIVE_GUARD_AUDIT', 'AUTOMATIC_NOCTURNAL'): the same set, with only the order changed;
    • the CASE ranks rows the same way as master's.

Standards

Pass-2 fixes: all three are fixed.

  1. The plan doc (lines 7 and 22) now says _CYCLE_VERDICT_PRIORITIES generates both fragments.
  2. Both fragments quote with f"'{k}'".
  3. TIER_COUNTERS is renamed ZERO_DAY_COUNTERS, with no references left to the old name.

There are no documented-standard violations, no new smells and no bugs.

Informational, not a finding: the generated CASE has a WHEN 'AUTOMATIC_NOCTURNAL' THEN 0 branch that repeats its ELSE 0. It's harmless, and it comes from generating both fragments from one map. No follow-up.

Spec

Fix claims (c3519): all true.

  • The plan doc and PR body describe the enforced contract, and disclose that the dict adds no new audit types, as #142's triage requires.
  • The PR description names the repository change, lists c3a06b9, 3a728b7 and 2744522, and gives the current 39 / 80 docs-check count.

#142 / #143:

  • #142.1–3: done. Item 3 is enforced in code.
  • #142.4: a no-op, per triage.
  • #143.1: done, with tests.
  • #143.2–3: no-ops, per triage. Both helpers are typed.

Closes #142, #143 is accurate. No scope creep.

Summary

  • Standards: 0 findings.
  • Spec: 0 findings.

Ready to merge.

🤖 Generated with Claude Code

## Code review, pass 3 (`origin/master...2744522`, spec #142, #143) Result: **no P1s, no P2s and no P3s.** This is the third pass of a follow-up PR, with nothing to fix and nothing to defer, so it **merges**. Verification: - ruff check and format: clean. - `check_docs.py`: 39 Markdown files and 80 HTTP operations checked, 0 errors. - `tests/test_cycle_verdict.py`, `test_statistics_marker_inputs.py` and `test_statistics_baselines.py`: 33 passed. - Wider set (`test_cycle_verdict`, `test_calibration_*`, `test_occupancy*`, `test_statistics_*`, `test_trust_dataset_parity`): 152 passed. - A SQL probe printed both generated fragments and compared them with master: - `IN ('RETROACTIVE_GUARD_AUDIT', 'AUTOMATIC_NOCTURNAL')`: the same set, with only the order changed; - the CASE ranks rows the same way as master's. ## Standards **Pass-2 fixes:** all three are fixed. 1. The plan doc (lines 7 and 22) now says `_CYCLE_VERDICT_PRIORITIES` generates both fragments. 2. Both fragments quote with `f"'{k}'"`. 3. `TIER_COUNTERS` is renamed `ZERO_DAY_COUNTERS`, with no references left to the old name. There are no documented-standard violations, no new smells and no bugs. *Informational, not a finding:* the generated CASE has a `WHEN 'AUTOMATIC_NOCTURNAL' THEN 0` branch that repeats its `ELSE 0`. It's harmless, and it comes from generating both fragments from one map. No follow-up. ## Spec **Fix claims (c3519):** all true. - The plan doc and PR body describe the enforced contract, and disclose that the dict adds no new audit types, as #142's triage requires. - The PR description names the repository change, lists `c3a06b9`, `3a728b7` and `2744522`, and gives the current 39 / 80 docs-check count. **#142 / #143:** - **#142.1–3:** done. Item 3 is enforced in code. - **#142.4:** a no-op, per triage. - **#143.1:** done, with tests. - **#143.2–3:** no-ops, per triage. Both helpers are typed. `Closes #142, #143` is accurate. No scope creep. ## Summary - **Standards:** 0 findings. - **Spec:** 0 findings. Ready to merge. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
gabogg merged commit 631072efd4 into master 2026-10-02 16:21:50 +00:00
gabogg deleted branch chore/statistics-review-followups 2026-10-02 16:21:50 +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!169
No description provided.