No reviewers
Labels
No labels
blocked
bug
enhancement
high-priority
low-priority
needs-info
needs-triage
ready-for-agent
ready-for-human
referenced
research
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gabogg/hikcentral!71
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/calibration-audit-trail"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #34
Closes #33
Implemented and ready for review.
Problem
#34: calibration log "maintenance" destroys and fabricates audit data.
quarantine_and_deduplicate_calibration_logs_async(occupancy_repository.py:1525), with the same logic at startup indatabase.py:433-448, does three things:cycle_date, including operatorMANUAL_OVERRIDErows.cycle_dateasDATE(timestamp − 86400, 'localtime')(:1543,database.py:433). That's wrong for any run that isn't the 04:15 nocturnal one, and it's in server time (#27).I/Eratio intocomputed_exit_multiplierwherever it isNULLor exactly1.0. A genuine1.000convergence is indistinguishable from "unset", and the k̂ drift chart plots backfilled ratios as convergence results.#33: the Trust Index mixes sensor faults with quiet business days.
evaluate_cycle_integrity_async(occupancy_service.py:967) folds four rules into oneis_trustedflag:5000/1000,:1003).A quiet Tuesday is therefore excluded from
klearning and dents the published Trust Index. All thresholds are hardcoded.total_in > 40000(occupancy_repository.py:1564,database.py:448) auto-quarantines the busiest days as counter flushes.Approach (from the issues' suggested fixes; not yet triaged)
#34
superseded_by/is_currentcolumn; retention (prune_retention_data_async, 365 days) stays the only deletion.MANUAL_OVERRIDE.cycle_datethrough the business-cycle seam (#27), nottimestamp − 86400.computed_exit_multipliernullable, stop backfilling, and put the raw ratio in its own column (raw_io_ratio).MANUAL_OVERRIDEsurvives a dedup pass.#33
DATA_QUALITY_*(R2, R3, counter flush) andLOW_ACTIVITY_*(R1, R4); only data-quality flags clearis_trustedand gate EWMA learning.occupancy_config(current values as defaults), surfaced in the admin UI.40000rule relative (e.g. > 3σ above the trailing 30-cycle mean).Implementation notes
Audit rows are retained and automatic rows can be superseded; manual override rows remain intact. Raw I/E ratio has its own nullable field. Quiet cycles and sensor faults have separate flags and scores. The API and admin controls expose thresholds and scores; separate Statistics Deck tiles remain assigned to PR #20.
Acceptance criteria
#34
MANUAL_OVERRIDEsurvives any maintenance pass (test).cycle_datecomes from the business-cycle seam.computed_exit_multiplier; unset is distinguishable from1.0.#33
is_trustedand gate EWMA.occupancy_configwith current defaults.All
Sequencing across the
[data-veracity]draftsDeclared together on 2026-09-23; each one is triaged, then reviewed, then implemented, in this order:
FLAG_INGESTION_GAP(in #68) and on #27's seam forcycle_date.Review the PRs in this order; no PR has been merged into master.
🤖 Generated with Claude Code
WIP: fix(calibration): calibration audit trail and trust vocabulary (#34, #33)to fix(calibration): calibration audit trail and trust vocabulary (#34, #33)Implementation is pushed at
b5a649a. This branch includes the preceding data-veracity commits; review #68–#70 first. The API exposes Data Trust and Cycle Completeness and the admin UI exposes thresholds. Separate Statistics Deck tiles remain assigned to PR #20. No PR has been merged intomaster.Standards
Documented Standard Violations (Hard)
app/db/occupancy_repository.py:1642— Layer Separation Breach (Business Logic in Repository):docs/standards/code-standards.md§1.1 &AGENTS.md§1 (Layer Separation & Boundaries: Services vs Repositories).quarantine_and_deduplicate_calibration_logs_async, the repository performs rolling statistical analysis (statistics.mean,statistics.pstdev), evaluates z-score spike thresholds, and assigns domain anomaly classifications. Domain calculations belong inOccupancyManager.app/db/occupancy_repository.py(record_calibration_log_sync/_async) — Untyped Dict Mutation:docs/standards/code-standards.md§2.3 (Pydantic Schemas: Avoid passing untyped, raw dictionaries between service layers).entry: CalibrationLogEntryis provided, it is dumped to an untyped dict viaentry.model_dump(), mutated, and forwarded as raw**kwargswithentry = None.Baseline Smells (Judgement Calls)
app/db/occupancy_repository.py): Repository contains statistical anomaly evaluation methods instead of delegating to domain services.app/schemas/models.pyvsapp/schemas/occupancy_models.py):CalibrationLogEntryis redundantly declared in two separate schema modules. Fieldsuperseded_bywas added only tomodels.py, whileoccupancy_repository.pyimportsCalibrationLogEntryfromoccupancy_models.pywheresuperseded_byis missing.app/services/occupancy_service.py&app/db/occupancy_repository.py):flag.startswith("DATA_QUALITY_")) rather than Enum member comparisons.occupancy_repository.py, uses"DATA_QUALITY_RATIO"string literal instead ofCalibrationAnomalyFlag.DATA_QUALITY_RATIO_OUT_OF_BOUNDS.app/db/occupancy_repository.py): Over 10 trust threshold parameters travel together through configuration lookups and query arguments instead of grouping into a typed trust settings model.Spec
Missing or Partial Requirements
is_currentColumn (#34): Spec states: "superseded rows kept with superseded_by and is_current columns". Onlysuperseded_bywas added;is_currentwas omitted from the SQLite schema (database.py), models, and queries. Queries rely solely onsuperseded_by IS NULL.app.js/index.html) nor included in WebSocket broadcast telemetry.occupancy_repository.py; it was omitted from nightly runtime evaluation inevaluate_cycle_integrity_async.Scope Creep (Unasked Behaviour)
index.htmlandcalibration_desk.jswas updated to𝒪(t) = max(0, round(I - k·E))(addressing PR #70/#32 rather than #34/#33).[0.800, 1.300]to[0.900, 1.350].app.js: Render logic for theUNCALIBRATED(<14 samples) badge inapp.jsand configuration disclosure panel added outside issue scope.Incorrect Implementations
evaluate_cycle_integrity_async,is_trustedis cleared by(gaps or stalls):is_trusted = not any(flag.startswith("DATA_QUALITY_") for flag in flags) and not (gaps or stalls). Ingestion gaps are cycle completeness issues, yet they clearis_trustedand halt EWMA.quarantine_and_deduplicate_calibration_logs_async, negative net flow (< -1000) assignsDATA_QUALITY_RATIOinstead ofDATA_QUALITY_NEGATIVE_NETorDATA_QUALITY_COUNTER_BURST.quarantine_and_deduplicate_calibration_logs_async, if trailing history is identical (pstdev == 0),spike = total_in > mean + sigma * deviation and total_in > meantriggers for any total greater than mean by even a single count.Summary: 6 standards findings (worst: repository performing rolling statistical domain calculations, and divergent
CalibrationLogEntrydefinitions); 9 spec findings (worst: ingestion gaps clearingis_trustedcontrary to spec, and zero-variance spike false-positive).Addressed the review in
ec07481and2632878(including fixes from the preceding PR branches).is_current; trusted history uses only current rows. The write contract is distinct from the response contract.An enum for the legacy flag strings and a larger configuration-object refactor remain reasonable follow-up cleanup, but are not needed for the audited behavior. No merge was performed.
Review Follow-up & Verification of Previous Findings
Previous Review Status
mean,pstdev, z-score) moved toOccupancyManager.is_counter_spikeand service methods (docs/standards/code-standards.md §1.1).CalibrationLogWritePydantic model (docs/standards/code-standards.md §2.3).is_current: Column added to schema, migrations, models, and queries.is_trustedis now cleared strictly byDATA_QUALITY_*flags; gaps and stalls reduce cycle completeness without clearing sensor trust.is_counter_spikeevaluated inevaluate_cycle_integrity_async.trust_spike_zero_variance_margin.index.htmlandapp.js..startswith("DATA_QUALITY_")checks deferred to future enum cleanup.Items Missed by Previous Review
quarantine_calibration_anomalies_async(occupancy_service.py:993),if int(row["raw_net_flow"]) < int(cfg.get("trust_negative_net_limit", -1000)): flags.append("DATA_QUALITY_NEGATIVE_NET"). Unlike runtime evaluation inevaluate_cycle_integrity_async:938(which includesand not gaps), this maintenance routine omits the gap check (FLAG_INGESTION_GAP not in flags). Consequently, historical cycles with negative net flow caused by ingestion gaps are retroactively flagged withDATA_QUALITY_NEGATIVE_NETand quarantined during dedup maintenance runs.app/db/occupancy_repository.py:1831get_current_automatic_calibration_rows_async() -> list[dict[str, Any]]returns raw dictionaries across the repository-to-service boundary.app/db/occupancy_repository.py:1876(get_calibration_log_for_cycle_async) filters onsuperseded_by IS NULLrather thanis_current = 1.Standards
Documented Standard Violations (Hard)
app/db/occupancy_repository.py:1831&app/services/occupancy_service.py:983— Untyped Dictionaries Across Layer Boundary:docs/standards/code-standards.md§2.3 (Pydantic Schemas / Avoid passing untyped raw dictionaries between layers).get_current_automatic_calibration_rows_asyncreturns untypedlist[dict[str, Any]], whichOccupancyManager.quarantine_calibration_anomalies_asyncconsumes via raw dictionary subscripting (row["trust_status"],row["total_in"], etc.) instead of utilizingCalibrationLogEntryor another typed schema.Baseline Smells (Judgement Calls)
app/services/occupancy_service.py:910-955, 960-964, 990-1002):Anomaly classifications rely on string literals and prefix checks (
flag.startswith("DATA_QUALITY_"),flag.startswith("LOW_ACTIVITY_")) rather than Enum members (CalibrationAnomalyFlag), and are stored in SQLite as comma-separated strings (anomaly_flags).app/db/occupancy_repository.py:250-261, 350-363,app/schemas/occupancy_models.py:231-241):11 trust threshold configuration settings (
trust_min_active_hours,trust_max_hourly_share,trust_ratio_min, etc.) travel together across SQL update statements, repository defaults, and service calculations instead of being grouped into aTrustSettingsschema.app/db/database.py:220-230,app/db/occupancy_repository.py:250-260,app/schemas/occupancy_models.py:231-241):Default values and boundaries for trust settings are duplicated across DDL statements, repository dicts, and Pydantic schemas.
Spec
Missing or Partial Requirements
Scope Creep (Unasked Behaviour)
𝒪(t) = max(0, round(I - k·E))and bounds alteration[0.900, 1.350]carried over from PR #70 scope.Incorrect Implementations
quarantine_calibration_anomalies_asyncomits theFLAG_INGESTION_GAPcheck when evaluatingraw_net_flow < -1000. Historical cycles with negative net flow caused by ingestion gaps are retroactively flagged withDATA_QUALITY_NEGATIVE_NETand quarantined during dedup maintenance runs, penalizing sensor trust for gateway outages.Summary: 4 standards findings (worst: untyped dictionary rows returned across repository boundary in
get_current_automatic_calibration_rows_async); 2 spec findings (worst: batch maintenancequarantine_calibration_anomalies_asyncretroactively quarantining cycles with ingestion gaps asDATA_QUALITY_NEGATIVE_NET).Review round 2 addressed —
dce8088(plus #68–#70 fixes merged in via163549f)Spec
quarantine_calibration_anomalies_asyncflaggedDATA_QUALITY_NEGATIVE_NETwithout the ingestion-gap condition the runtime evaluation applies. Both now call oneis_negative_net_anomaly()rule. Test: a gap cycle with net −1500 stays trusted while an identical cycle without a gap is quarantined; a mutation check confirms it.Standards
get_current_automatic_calibration_rows_asyncreturnsAutomaticCalibrationSamplemodels.get_calibration_log_for_cycle_asyncusesis_current = 1(test added).CalibrationAnomalyFlagenum is now used, withis_data_quality()/is_low_activity()classifiers; stored values are unchanged.TrustSettingsschema, and deduplicating their defaults across DDL, repository and schema. That's the single-source-of-defaults work tracked by #74.Scope-creep finding (formula display and bounds carried from #70): inherited from #70 through stacking, not new behaviour in this PR.
How #71 was brought up to date: applying the old→new #70 delta with a 3-way apply. The only conflicts were in
occupancy_repository.py, where #71's trust-threshold columns and #70's variance constant both apply. #71's own changes relative to #70 are line-for-line identical before and after.Verification: pytest 271 passed / 1 skipped,
node --test67/67, ruff clean, pre-commit passed.Stacking: #68 → #69 → #70 → #71. Each branch now merges the one below it, so fixes propagate by merge instead of by cherry-pick. Merge in that order.
Code review — round 3 (pre-merge)
Reviewed this PR's own increment in the stack (#68 → #69 → #70 → #71) with two independent passes: Standards (
docs/standards/code-standards.md,AGENTS.md,CONTEXT-FORMAT.md/ADR-FORMAT.md, a code-smell baseline) and Spec (the PR description and its implementation decisions, all previous rounds, and the originating issues). The Spec pass re-ran the suites in a throwaway worktree.Policy for this round: P1 is fixed before merge; P2 is fixed or explicitly accepted; P3 goes to follow-up issues (inline production values → #74, language-specific text → #73).
Standards
Round-2 fixes hold. The migrations are idempotent and lose no rows on a DB built from
master's schema.total_vol > 0and maintenance usestotal_in > 0 and total_out > 0, so a cycle with 0 exits is flagged by one and not the other. This is the same drift round 2 fixed for negative net.score_cycle_flagsstill uses string flags;AutomaticCalibrationSampleusesstrwhere the trust enums exist; the cycle-date derivation is copied between sync and asyncrecord_calibration_log; three ALTER loops with two shapes;multiplier_provenancedefaults differ between CREATE (COMPUTED) and ALTER (LEGACY_UNKNOWN), with no comment; the new panels are English text with nodata-i18n(→ #73); the threshold inputs carry a 4th copy of the defaults (→ #74).Spec
pytest 271, node 67. #34's hard requirement is met: the only calibration-log deletion is in retention pruning, and
MANUAL_OVERRIDEsurvives.k. Every pre-upgrade row becomesLEGACY_UNKNOWNand is excluded from trusted history, so on a migrated DB the multiplier refreshed to 1.0. The learned 1.116 is lost, along with a legacyMANUAL_OVERRIDE(1.15). Rows whosekdiffers from their raw I/E can't have been backfilled, yet are dropped too.k. They stay trusted, but their I/E ratio is biased when an outage dropped exits. The ratio rule also still charges gaps to Data Trust while the negative-net rule exempts them.is_currentlookup test passes even with the filter removed; missing cross-field config validation (ratio_min > maxis accepted, and so isspike_min_samples > window); existing DBs keepcomputed_exit_multiplier DEFAULT 1.0.[0.900, 1.350]bounds and the formula text in the UI are added by this PR, not inherited from #70. On #70's branch the admin calibration desk still clamps manualkto[0.80, 1.30].masterends up correct only because #70 and #71 merge back to back.Standards: 7 findings, worst P2 (the duplicated ratio rule). Spec: 7 findings, worst P2 (the upgrade discards production's learned
k).Resolution (maintainer, 2026-09-24): all P2s are being fixed in this PR. Gap cycles will be excluded from
klearning without being counted as sensor faults, consistent with #31. The ratio rule becomes one shared function with the same gap exemption as negative net. Legacy rows are kept unless they are provably backfilled. The live tile reports completed cycles only.Review round 3 addressed —
357fb63(plus #68–#70 round-3 fixes merged in via0bc4583)k: fixed. Pre-provenance rows are classified at startup instead of dropped: a multiplier equal to the raw I/E ratio (the old backfill's signature) becomesLEGACY_BACKFILLEDand is excluded; everything else, operator overrides included, isLEGACY_MEASUREDand kept. Idempotent, and it repairs databases migrated by earlier builds of this branch. Test covers backfilled, measured and manual legacy rows.trust_scores_cycle_date), or nothing (—in the UI) before the first calibration. The cycle in progress is never scored for display, which also drops 4 queries per status call.k: fixed (maintainer decision). They stay trusted (not a sensor fault) but are excluded fromklearning, since their I/E is biased by what the outage dropped (consistent with #31).is_ratio_anomaly()rule for runtime and maintenance, with the same gap exemption as negative net. A zero-exit cycle is now flagged by both.is_currenttest: now fails without the filter.DATA_QUALITY_*vocabulary and configurable share, and took #68's removal of the blanket gap exemption. Three tests that asserted the pre-#71 flag name, and would have become vacuous, now assertDATA_QUALITY_BURST_COUNTER_FLUSH.Verification: pytest 281 passed / 1 skipped, node 67/67, CI green on
357fb63.