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!45
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/hikctl-path-registration-and-db-init"
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 #41
Closes #43
Closes #48
Overview & Scope
Database lifecycle, CLI provisioning ergonomics, credential hygiene, and the door-exclusion persistence invariant. System
PATHregistration (#23) was spun out into #47 following review, to isolate platform init and Windows registry work.This description reflects the final plan settled during the draft phase (reviews: #943, #952).
hikctl db init: lightweight schema/migration creation with no default-user seeding and no door-flag mutation.cmd_user.pyathikctl db initandhikctl setup.--seed-defaultsseeds default accounts only into an emptyuserstable; refuses otherwise, so deleted accounts can't be resurrected.DEFAULT_SEED_USERSinapp/db/database.py; tests import it instead of hardcoding passwords.Technical Specification
1. #41 — lightweight
hikctl db init& seeding safetyapp/db/database.py):init_schema(conn=None): tables, views, covering indexes, WAL (PRAGMA journal_mode=WAL; PRAGMA synchronous=NORMAL;), and historical migrations (e.g.cycle_datebackfill, calibration-log anomaly quarantine). Zero user seeding, zero door-flag mutation.seed_default_users(conn=None, allow_non_empty: bool = False): seeds fromDEFAULT_SEED_USERS; whenSELECT COUNT(*) FROM users > 0andallow_non_emptyisFalse, logs and returns without inserting — no resurrection of deleted defaults.init_db(): retained for FastAPI lifespan (app/main.py:35); callsinit_schema()thenseed_default_users(allow_non_empty=False). It no longer resets any door flags (see §3).app/cli/commands/cmd_db.py,app/cli/main.py): addhikctl db initunder the existingp_dbparser.--seed-defaults: explicitly seed defaults; refuses with an informative error if any users already exist.--skip-schemadropped (speculative generality).cmd_user.py):"Database is not initialized. Run 'hikctl db init' to create the schema, or 'hikctl setup' for full host provisioning."2. #43 — module-level seed credential constant
DEFAULT_SEED_USERS: list[tuple[str, str, str]] = [("admin", "ControlHG", "admin"), ("operator", "OperadorHG", "operator")]inapp/db/database.py.tests/test_cli_commands.py::test_user_command_does_not_resurrect_deleted_userimports the constant and resolves the admin seed dynamically — no password literal in tests.3. #48 — persistent door exclusions (AUTO/MANUAL provenance)
exclude_from_rankingsis a single flag with no provenance, so the auto-recovery reset for sensorless doors also wipes operator exclusions on restart and telemetry sync. Fix by recording the source, not by deleting the reset:app/db/database.py): addexclusion_source TEXT DEFAULT ''('AUTO'|'MANUAL'|'') todoor_records, in both theCREATE TABLEand the in-placePRAGMA table_infomigration (same additive pattern asis_tracked/tracking_status,database.py:72-87). Backfill excluded rows by category:VERIFIED_SENSOR→MANUAL,SENSORLESS_*→AUTO.app/db/door_repository.py): sensorless auto-exclude writesAUTO;set_exclusion_sync/set_category_exclusion_syncwriteMANUAL;upsert_synccarriesexclusion_sourcethrough so syncs don't drop it.VERIFIED_SENSORtransition clears the exclusion only when source isAUTO;MANUALpersists. Remove the blanket startupUPDATE ... WHERE category = 'VERIFIED_SENSOR'(database.py:385) anddoor_repository.py:88-89.4. Documentation
CONTEXT.md§2: documenthikctl db initlifecycle and the AUTO vs MANUAL exclusion sources.docs/guides/server-cli-operations.md,docs/architecture/server-setup-lifecycle-and-monitoring.md:db init/--seed-defaultsrunbook entries.Acceptance Criteria
hikctl db initcreates schema, migrations, and WAL without seeding accounts or modifying door records.hikctl db init --seed-defaultsseeds defaults on an empty DB; refuses with an error if users already exist.hikctl db init→hikctl user addsucceeds.hikctl useruninitialized-database error points tohikctl db initandhikctl setup.DEFAULT_SEED_USERS.door_recordsgainsexclusion_source, viaCREATE TABLEand in-place migration; existing DBs upgrade without loss; excluded rows backfilled by category.MANUAL) exclusions onVERIFIED_SENSORdoors persist acrossinit_db()restart and across a telemetryupsert_syncthat omits the flag.VERIFIED_SENSORre-enters rankings (AUTOcleared).UPDATEanddoor_repository.py:88-89removed, replaced by the source-aware rule.test_user_command_does_not_resurrect_deleted_user,test_user_command_does_not_wipe_door_exclusions) still pass.CONTEXT.mdand runbooks updated.🤖 Generated with Claude Code
🔍 Draft Review —
feat/hikctl-path-registration-and-db-initNo code to review yet
Head
bbc435ais byte-identical tomaster:The
WIP:prefix is correct per AGENTS.md §4. What follows reviews the plan in the description against #23, #41 and #43. The two-axis Standards/Spec review needs a non-empty diff and will run once commits land.Plan vs. the three issues
#23 — PATH registration
The plan covers both acceptance criteria. Two gaps:
~/.local/binfallback silently fails the issue's own AC. #23 requires: "It must support global availability so administrative shells and background orchestration have access." A user-scoped symlink is not on a systemd unit's or a scheduled task'sPATH. The unprivileged path should warn clearly that the installation is user-scoped rather than reporting success.setupphase". It is a sensible addition; just flagging it as a deliberate extension rather than a requirement.#41 — lightweight
hikctl db initThe seam fits cleanly:
p_db/db_actionalready exists atapp/cli/main.py:154-155next tostatus/checkpoint/vacuum/integrity/backup, soinitslots in without restructuring.Two issues:
hikctl user addsucceeds." The plan's AC list covers each flag individually but never the round trip. That end-to-end path is the one that provesdb initactually replaceshikctl setupfor provisioning the first account — it is the reason the issue exists.--skip-schemais speculative generality. Adb initthat skips table DDL only means "seed users into an existing database", which wants to be eitherdb seedor simply--seed-defaultsused on its own. #41 never asked for it, and its stated AC — "prints an explicit warning" — asserts a warning string rather than behaviour. Suggest dropping it until a caller needs it.#43 — seed credential constant
Satisfied as planned. One residual worth naming:
DEFAULT_SEED_USERSstill placesControlHGin application source, and promoting it to a named module-level constant makes it more discoverable, not less. #43 only asked for no literal in the test suite, so the plan meets the ask — but the durable fix is generating a random password at seed time and printing it once, the way provisioning tools normally do. That belongs in its own issue rather than expanding this PR.Design risks
Windows Machine PATH is the risky piece.
[Environment]::SetEnvironmentVariable("PATH", ..., "Machine")is a well-known footgun: the .NET getter expandsREG_EXPAND_SZvalues, so a read-modify-write rewrites%SystemRoot%\system32as a literal expanded path and changes the value type toREG_SZ. On a production Windows Server that is a destructive change that will not be noticed until something else breaks. Read the raw value throughMicrosoft.Win32.RegistrywithDoNotExpandEnvironmentNamesand write back preservingREG_EXPAND_SZ. BroadcastWM_SETTINGCHANGEafterwards, or the newPATHwill not be visible until reboot.Uninstall should remove only what setup created. Unlinking
/usr/local/bin/hikctlwithout first confirming the symlink resolves into our own bin directory, or stripping aPATHentry a sysadmin added by hand, is destructive. Verify ownership before removing in both adapters.--seed-defaultspartially re-opens the hole closed in #39. The regression fixed there was default-credential accounts reappearing behind the operator's back. An explicit opt-in flag is defensible, but run against an existing database whereadminwas deliberately deleted, it will resurrect that account with the default password. The command should refuse, or at minimum warn, when it is about to recreate an account that was previously removed.⚠️ The decision that matters most
The plan keeps
reset_stale_verified_door_flags()insideinit_db()"for backward compatibility (FastAPI app lifespan)". That codifies a bug that is already live onmaster.Operator-set exclusions on
VERIFIED_SENSORdoors do not survive normal operation today. Two independent paths clear them, both reproduced againstorigin/master:Path 2 is this branch in
upsert_sync:Any upsert that omits the flag — which routine telemetry syncs do — clears it.
The net effect is that the exclusion feature delivered by #25 / #37 is non-persistent for exactly the door category operators are most likely to quarantine. This predates #45 and is not caused by it. It matters here because #45 is where the decision to preserve that reset is being taken, and "backward compatibility" is the wrong frame for keeping it.
Recommend splitting this into its own issue and settling it before or alongside #45: both
db initsemantics and--seed-defaultsdepend on whatinit_db()is permitted to mutate.Scope
Three unrelated issues on one branch is Divergent Change and leaves poor revert granularity. #43 is a two-line test change that could merge today; #23's Windows PATH work needs careful review and realistically a Windows Server to validate against. Splitting #43 out would unblock it immediately.
Summary — no diff to review; plan-level findings: 2 gaps against #23, 2 against #41 (one missing AC, one speculative flag), 1 residual on #43, 3 design risks, and 1 pre-existing bug on
masterthat this plan would entrench. Nothing blocking a draft; the door-exclusion persistence question should be settled before theinit_db()refactor is written.🤖 Generated with Claude Code
WIP: feat(cli): system PATH registration, lightweight db init, and credential hygiene (#23, #41, #43)to WIP: feat(cli): lightweight db init, credential hygiene, and persistent door exclusions (#41, #43)🛠️ Draft Plan Remediation & Responses to Review
Thank you for the thorough and incisive review. Following a structured design interview, we have adopted your recommendations in full across all points.
Here is the remediation summary:
1. Scope & Decomposition (Divergent Change)
feat/hikctl-system-path-registration), isolating platform init and Windows registry logic.hikctl db init), #43 (seed credential constant extraction), and resolving the door ranking exclusion persistence bug uncovered during review.2. Settling "The Decision That Matters Most" (Door Ranking Exclusion Bug)
UPDATE door_records SET exclude_from_rankings = 0 WHERE category = 'VERIFIED_SENSOR'fromapp/db/database.py. It will not be preserved ininit_db().app/db/door_repository.py(elif category == "VERIFIED_SENSOR" and existing["exclude_from_rankings"] == 1: exclude_from_rankings = 0). Subsequent telemetry syncs will strictly preserve existingexclude_from_rankingsvalues.CONTEXT.md): Updating Section 2 to crystallize that operator-set door exclusions are persistent across server restarts and routine telemetry syncs.3. Issue #41 Refinements (
hikctl db init)--skip-schema: Removed speculative flag entirely per review feedback.fresh DB -> hikctl db init -> hikctl user add succeeds.--seed-defaultsonhikctl db initis strictly guarded: it will checkSELECT COUNT(*) FROM usersand refuse execution with an error if existing accounts are found, preventing accidental recreation of deletedadmin:ControlHG.init_db()during FastAPI lifespan boot, preventing resurrection upon server restarts.4. Issue #43 Refinements (Credential Hygiene)
DEFAULT_SEED_USERSconstant extracted inapp/db/database.pyand dynamically referenced intests/test_cli_commands.pyteardown fixture.The PR description above has been updated to reflect the remediated plan and acceptance criteria. We welcome any follow-up review on this updated plan before code implementation commences!
🔍 Draft Review (round 2) —
feat/hikctl-path-registration-and-db-initStill no code — and the branch is stale
The remediation updated the description and posted a response, but no commits landed. The branch head is
bbc435a, which is now one commit behindmaster(1bb097d, the.gitignorefix) — zero commits ahead:So this reviews the updated plan. When code starts, rebase the branch onto
masterfirst, or theupdate applygitignore fix won't be present. (The branch name still sayspath-registration, which now lives in #47 — cosmetic, but rename when convenient.)What the plan got right
Adopts the prior review cleanly: #23 split into #47,
--skip-schemadropped, the missingfresh DB → db init → user addroundtrip AC added, and the--seed-defaultsresurrection loophole closed with a zero-rows guard. Good.🔴 The door-exclusion fix, as specified, trades one bug for another
The plan removes both resets outright:
UPDATE door_records SET exclude_from_rankings = 0 WHERE category = 'VERIFIED_SENSOR', anddoor_repository.py:88-89(elif category == "VERIFIED_SENSOR" and existing["exclude_from_rankings"] == 1: exclude_from_rankings = 0).That fixes the reported bug (operator exclusions surviving restarts and syncs). But those lines are not only the operator-exclusion wipe — they are the auto-recovery path for sensorless doors, and deleting them blanket strands recovered doors.
Here is the coupling:
filter_sensorlessdefaults toTrue(app/config.py:45).SENSORLESS_OPEN/SENSORLESS_JUMPEREDdoor is auto-excluded at insert (door_repository.py:68-74).door_classifierlater re-classifies that door back toVERIFIED_SENSOR(sensor recovered), line 88-89 is what clears the auto-exclusion so the door re-enters rankings.Delete 88-89 with no replacement and an auto-excluded door that recovers to
VERIFIED_SENSORstays excluded forever — a new bug pointing the opposite way from the one being fixed.Root cause of the dilemma:
exclude_from_rankingsis a single boolean with no provenance. There is no way to tell "operator quarantined this door" from "system auto-hid this sensorless door" — bothset_exclusion_sync(door_repository.py:441) and the sensorless auto-exclude write the same flag. The reset can't distinguish them, so any rule over that one bit is wrong for one of the two sources.Recommendation: add provenance before deleting the resets. e.g. an
exclusion_sourcecolumn ('AUTO'|'MANUAL') or a second boolean. Then:VERIFIED_SENSORtransition) clears onlyAUTOexclusions;MANUAL) exclusions persist across restarts and syncs — the invariant #45 wants.That is a schema migration plus touching
set_exclusion_sync/set_category_exclusion_sync/ the sensorless auto-exclude, so it is more than "delete two lines and one UPDATE". It is the honest scope of "persistent door exclusions". If you want to keep this PR small, splitting the exclusion invariant into its own issue (with the provenance design) and landing #41/#43 alone here is reasonable — but do not ship the blanket deletion as-is.Smaller notes
seed_default_usersseeding only whenCOUNT(*) == 0means an operator who deleted just theoperatoraccount but kept a custom admin can never re-seed the defaultoperatorviadb init --seed-defaults(table is non-empty → refused). Acceptable —hikctl user addcovers it — but state it in the command's help so it is not surprising.Verdict
Plan-level: #41 and #43 are ready to implement as written. The exclusion invariant is directionally right but under-specified — as drawn it regresses sensorless-door recovery. Settle provenance first. Nothing to run yet; will do the two-axis review once commits exist.
🤖 Generated with Claude Code
Opened #48 for the door-exclusion provenance problem, and it should be resolved within this PR rather than split out — #45 already owns exclusion persistence, so folding the provenance model in here keeps the domain change and its migration in one place.
Action for this PR: replace plan item 5 ("Door Ranking Exclusion Invariant") — do not blanket-delete the startup
UPDATEanddoor_repository.py:88-89. Instead implement #48's approach:exclusion_source(AUTO|MANUAL) todoor_records, via both theCREATE TABLEand the in-placePRAGMA table_infomigration;AUTO; operatorset_exclusion_sync/set_category_exclusion_syncwriteMANUAL;VERIFIED_SENSORtransition clears onlyAUTO; operatorMANUALexclusions persist across restarts and telemetry syncs.Full design, migration/backfill details, and acceptance criteria are in #48. That AC set should be merged into this PR's checklist. The
Closes #41,Closes #43on this PR should gainCloses #48.Everything else in the round-2 review (#41 roundtrip AC,
--seed-defaultsguard, #43 constant) stands unchanged.WIP: feat(cli): lightweight db init, credential hygiene, and persistent door exclusions (#41, #43)to feat(cli): lightweight db init, credential hygiene, and persistent door exclusions (#41, #43, #48)hikctl db initinstead of pointing operators at full host provisioning #41🔍 Two-Axis Code Review —
feat/hikctl-path-registration-and-db-initBase
1bb097d(master) → head0a247ad0. One commit, 11 files, +435/−51. Spec: #41, #43, #48.Reviewed along two independent axes (Standards and Spec) so neither masks the other.
Standards
Hard violations
1. Inline SQL outside
app/db/—app/cli/commands/cmd_db.py:41-46The
db init --seed-defaultsguard opens a raw connection and runs SQL in the CLI layer:Violates code-standards §1.1.3 ("All SQL strings must reside within repository classes") and AGENTS.md §1.3 / §3 ("no inline SQL in services or controllers … must run through repository methods"). CLI commands are the thin interface layer. Avoidable:
seed_default_users(allow_non_empty=False)already performs the empty-table check and returns a bool. The command re-implements that guard in raw SQL, then callsseed_default_users(allow_non_empty=True)to disable the very guard it just duplicated — also a Duplicated Code / Middle Man smell; the emptiness gate now lives in two places and can drift.Baseline smells (judgement calls)
exclusion_sourceas barestr. The AUTO/MANUAL/'' provenance is a raw string with literals repeated acrossdatabase.py(CREATE, migration CASE),door_repository.py(_prepare_record_valuesbranches,set_exclusion_sync/async,set_category_exclusion_sync), andmodels.py. The repo already favors enums (OpenTrigger). AnEnum/Literalwould prevent the exact boolean↔source drift this PR exists to fix — a single typo ("MANAUL") silently defeats persistence. Centralize the values.door_repository.py:_prepare_record_values(~65-115).exclude_from_rankingsandexclusion_sourceare set together across ~7 branches (3 new-record, 4 existing-record). Logic is correct and mirrors the spec (AUTO cleared only on VERIFIED_SENSOR + existing AUTO; MANUAL persists), but the pairing is hand-maintained in every branch — a helper returning(flag, source)removes the drift surface. Migration is idempotent, guarded byif "exclusion_source" not in existing_cols(matches theis_trackedpattern); blanket startup UPDATE and old reset removed. No correctness issue.models.py:163exclusion_source: str = Field(default="", alias="exclusion_source"): alias equals field name, a no-op (Speculative Generality).AuditItem.exclusion_sourceuses noField, so the two are inconsistent.-> Nonehints onset_exclusion_sync/async,set_category_exclusion_sync— signatures untouched by this PR, so not a new violation; new functions (init_schema,seed_default_users,init_db) are fully annotated per §2.2.Test-code standards
Deterministic, offline, real sqlite (no over-mocking) — §4 compliant.
test_door_exclusion_migration_backfillbuilds a legacy schema and asserts backfill by category. Minor inconsistency:test_..._provenance_and_persistenceruns against the shared test DB and calls globalinit_db()while the migration test monkeypatchesdb_path— style nit, not a violation. Coverage matches the #48/#41 ACs.Spec
#48 provenance (highest-risk item) — verified correct end to end
CREATE TABLE(database.py:72exclusion_source TEXT DEFAULT '') and the in-place migration (database.py:84-93,PRAGMA table_info-guardedALTER). Backfill CASE:VERIFIED_SENSOR→MANUAL, else (SENSORLESS_*)→AUTO, onlyWHERE exclude_from_rankings=1 AND (source IS NULL OR '')— matches the AC.door_repository.py): sensorless auto-exclude writesAUTO(:76-78);set_exclusion_sync/set_category_exclusion_sync/set_exclusion_asyncwriteMANUALon exclude,''on clear (:469,:485,:498);upsert_synccarriesexclusion_sourcethrough insert/update SQL and the record mapping (:158,:271,:334).door_repository.py:104-113):elif category=='VERIFIED_SENSOR' and existing_excluded==1 and existing_source=='AUTO'clears; else preserves. Matches "clears only AUTO; MANUAL persists".UPDATE … WHERE category='VERIFIED_SENSOR'gone (0 hits in app/); olddoor_repository.py:88-89reset gone.init_db()(database.py:453-455) =init_schema+seed_default_usersonly, no door mutation; still wired atapp/main.py:35.else(:110-113) preservingexisting_excluded=1, MANUAL. Holds — assertedtest_repositories.py:207-227.exclude=0, source=''. Holds — assertedtest_repositories.py:240-261.#41 db init
hikctl db init(cmd_db.py:31-56, parsermain.py:156-165) callsinit_schema()only — no seeding/flag mutation.--seed-defaultsguardsSELECT COUNT(*) FROM users, refuses with exit 1 if non-empty, else seeds. Error guidance (cmd_user.py:213) names bothhikctl db initandhikctl setup. Roundtrip + guard tests present.--skip-schemacorrectly absent.#43 credential hygiene
DEFAULT_SEED_USERSatdatabase.py:13-16; test imports it and resolves the admin pwd dynamically (test_cli_commands.py:890-892) — no"ControlHG"literal in tests.Scope creep
None material.
set_exclusion_asyncandmodels.py:163,398(exclusion_sourceon DoorEntity/AuditItem) are legitimate #48 carry-through. PATH work (#23) correctly absent — spun to #47.Test evidence
python -m pytest -q→ 201 passed, 1 skipped, 0 failed. Targetedtest_repositories.py test_cli_commands.py→ 44 passed. New tests assert the ACs, not just implementation: both #48 invariants + migration backfill (with a genuine legacy no-column DB) and the seed guard both directions. Minor gaps (non-blocking): (a)test_user_command_uninitialized_databaseasserts only the"hikctl db init"substring, not"hikctl setup"(message contains both — AC met but half-asserted); (b) thenode --testhalf of the "full suite green" AC wasn't run here (JS unchanged bar a doc reference).Summary — Standards: 1 hard violation (inline SQL in
cmd_db.py:41-46, which also duplicates a guardseed_default_usersalready provides) + 4 judgement calls; worst is that hard violation. Spec: all ACs of #41/#43/#48 hold, including both #48 provenance invariants proven by tests; no missing/wrong requirements, negligible scope creep — worst is a half-asserted error-message test. The Standards hard violation and the Spec pass are consistent: the seed guard works, it's just implemented in the wrong layer.🤖 Generated with Claude Code
✅ Addressed Review Feedback
Inline SQL Removal & Guard Deduplication (
app/cli/commands/cmd_db.py):SELECT COUNT(*) FROM usersfromcmd_db.py.seed_default_users(allow_non_empty=False)fromapp.db.database, eliminating the duplicated emptiness gate and adhering to architectural boundaries.Primitive Obsession Resolution (
app/schemas/models.py,app/db/door_repository.py,app/db/database.py):ExclusionSource(str, enum.Enum)(AUTO,MANUAL,NONE = "").ExclusionSource.alias="exclusion_source"inDoorEntity.Duplicated Code / Drift Guard (
app/db/door_repository.py):determine_door_exclusion(incoming_data, existing, category, filter_sensorless)helper to encapsulate paired assignment of(exclude_from_rankings, exclusion_source)across new and existing records.test_determine_door_exclusion_helpercovering all decision branches intests/test_repositories.py.Type Annotations:
-> Nonereturn type hints toset_exclusion_sync,set_exclusion_async, andset_category_exclusion_sync.🔁 Re-review — verification of review response (
0a247ad→02beef0)Base
1bb097d(master) → head02beef0. Two commits. Scope of this pass: confirm the fixes claimed in #1045 actually landed and are correct, and check what the first review (#1019) missed.Baseline re-run independently:
ruff checkclean ·ruff format --check109 files formatted ·pytest202 passed, 1 skipped ·node --test59/59 · merges cleanly with #47 despite 4 shared files.Standards
Prior review items — verdicts
cmd_db.pyseed_default_users(allow_non_empty=False)exclusion_source.value, anddetermine_door_exclusionreturnstuple[int, str]. It's a constant namespace, not a type; the concept still travels asstrthrough the repositorycategoryis in scope at the new call site (door_repository.py:113assigned before:121)alias="exclusion_source"-> Nonehintsdoor_repository.py:490, 501, 512)test_door_exclusion_migration_backfillmonkeypatchesdb_path, buttest_door_exclusion_provenance_and_persistencestill drives the shared autouse DB and calls globalinit_db()mid-test, leaving rows behindMissed by the first review
1.
app/schemas/models.py:169+app/db/door_repository.py:210— the new field is dead._format_resultemits camelCase"exclusionSource", whileDoorEntity.exclusion_sourcecarries no alias — every sibling does (lastStateChange,doorIndexCode, …). Confirmed empirically:AuditItem.exclusion_sourceis worse:_build_audit_report(door_service.py:1040+) never sets the key at all, soSensorAuditResponseships a permanently-empty value on every door. A grep forexclusionSourceacrossapp/,app/static/andtests/finds zero consumers.No user-visible break today — but the whole model-layer half of #48 is currently decorative, and the snake/camel mismatch is a trap for whoever first tries to read it. Note the alias was snake_case before
02beef0too, so removing it didn't cause this; it was inherited from0a247adand left unfixed. Suggested:Field(default=ExclusionSource.NONE, alias="exclusionSource"), and populate it in_build_audit_report.Related:
record.get("exclusion_source", "")(door_repository.py:210) returnsNoneon a NULL column — the migration's ownIS NULLguard admits NULL is reachable. Preferrecord.get("exclusion_source") or "".2.
app/db/database.py:453-456— f-string SQL. The backfillUPDATEinterpolatesExclusionSource.*.valuedirectly. Trusted constants, so not injectable, but code-standards §1.1.3 asks for parameterized queries and?placeholders cost nothing here.3. Layer placement of the extracted SQL.
seed_default_userslives inapp/db/database.pyas a free function, not a repository class — §1.1.3 says SQL belongs in repository classes, anduser_repository.pyalready ownscreate_user/hash_password. The inline SQL was correctly removed from the CLI; it just didn't land in a repository.4. Function-local imports in the new
test_determine_door_exclusion_helper— the same nit #47 was asked to fix, and did.Layering is otherwise fine:
app/db→app/schemasis pre-existing,models.pyimports noapp.*, no cycle.Spec
All ACs for #41 / #43 / #48 still hold at
02beef0— the refactor preserved behaviour:seed_default_users's bool is sound for the CLI's call path: refusal returnsFalseimmediately (count > 0 and not allow_non_empty); on an empty table every seed row misses theSELECT idcheck and inserts, soseeded=True. No falsy-on-success, no truthy-on-refusal.MANUAL/''; sensorless +filter_sensorlesson a new row →AUTO).UPDATEand the olddoor_repository.py:88-89reset remain gone.WHEN category = 'VERIFIED_SENSOR' THEN 'MANUAL' ELSE 'AUTO'.CONTEXT.md§2 and both runbooks carry thedb init/--seed-defaultsand AUTO-vs-MANUAL entries.Missed by the first review
1. Scope creep — incoming telemetry can set provenance.
door_repository.py:35-39:#48 asks only that
upsert_sync"carryexclusion_sourcethrough … so routine syncs don't drop it" — preserve, not accept an override. Consequences: a sync payload carryingexclude_from_rankings=1, exclusionSource="AUTO"re-labels an operator exclusion as auto-clearable, directly weakening the "MANUAL persists" invariant this PR exists to establish. There is also no enum validation on the way in, so an arbitrary string can reach the column and then failExclusionSourcevalidation on the way out. Present since0a247ad. Suggest restricting toExclusionSourcemembers, or dropping the override entirely.2. Backfill
ELSEis broader than spec. #48 says "SENSORLESS_*→AUTO", but theCASEsends every non-VERIFIED_SENSORexcluded row toAUTO. Harmless against today's category set; a future category would be silently auto-recoverable.3.
seed_default_users's return is overloaded — withallow_non_empty=Trueand defaults already present it returnsFalsewithout having refused. OnlyFalseis passed today so nothing breaks, butcmd_db.py:42prints "users table is not empty" for a bool that also means "nothing was inserted". An explicit outcome would be sturdier than a bool.Correction to the first review
The first review flagged
test_user_command_uninitialized_databaseas asserting only the"hikctl db init"substring. That was wrong —tests/test_cli_commands.py:885-886asserts both"hikctl setup"and"hikctl db init". That AC was fully covered all along; no action needed.Summary — Standards: 6 prior items → 3 fixed, 2 partial, 1 fixed-but-exposing; 4 new findings, 0 hard violations remaining. Worst: the
exclusionSource/exclusion_sourcealias mismatch leaves both new model fields permanently unpopulated. Spec: all ACs hold; 3 new findings + 1 correction in the PR's favour. Worst: incoming telemetry can override exclusion provenance, undercutting the invariant #48 exists to guarantee.🤖 Generated with Claude Code
Response to Re-Review (
02beef0→bffb424)Addressed all findings from the follow-up review:
Alias Mismatch on
exclusion_sourceResolved:DoorEntityandAuditIteminapp/schemas/models.pyboth configureexclusion_source: ExclusionSource = Field(default=ExclusionSource.NONE, alias="exclusionSource")withConfigDict(populate_by_name=True)._build_audit_reportinapp/services/door_service.pysets bothexclusion_sourceandexclusionSource.test_door_entity_and_audit_item_exclusion_source_alias).Domain Strong Typing in
determine_door_exclusion:tuple[int, ExclusionSource].ExclusionSource.AUTO,ExclusionSource.MANUAL, andExclusionSource.NONE._coerce_exclusion_sourcehelper to validate incoming and database values safely against the domain enum.Scope-Creep Guard (Downgrade Prevention):
determine_door_exclusion: when an existing record hasExclusionSource.MANUALandflag == 1, incoming sync payloads cannot downgrade the source toAUTOor any arbitrary string.exclusion_source="AUTO"preservesExclusionSource.MANUALwhenexclude_from_rankingsremains active.Layering & Repository Architecture:
app/db/database.pyintoUserRepository.seed_default_users(conn, allow_non_empty)inapp/db/user_repository.py.DEFAULT_SEED_USERSindatabase.pyfor backward compatibility, withdatabase.seed_default_userscleanly delegating touser_repo.seed_default_users.Parameterized Schema Migration:
init_schemawith parameterization (?).CASEbackfill toWHEN category = ? THEN ? WHEN category LIKE 'SENSORLESS_%' THEN ? ELSE ''per #48 spec.Test Isolation & Hygiene:
tests/test_repositories.py(sqlite3,init_schema,determine_door_exclusion,ExclusionSource,SensorCategory) to module-level imports.test_door_exclusion_provenance_and_persistencewithtmp_pathandmonkeypatchtargeting an isolated test database.🔁 Third pass — verification of round-2 response (
02beef0→bffb424)Base
1bb097d→ headbffb424. Scope: confirm the fixes claimed in #1055 landed correctly, and check what #1047 missed.Baseline re-run independently:
ruff checkclean ·ruff format --check109 files formatted ·pytest203 passed, 1 skipped ·node --test59/59 · merges cleanly with #47.Standards
determine_door_exclusionUserRepositoryClaim 1 holds under direct probe. Both models carry
alias="exclusionSource"withpopulate_by_name=True; both spellings validate; an out-of-enum value raisesValidationError:The previously-dead field is genuinely populated now.
New findings
1.
_coerce_exclusion_sourceswallows invalid values silently (app/db/door_repository.py).The enum was introduced specifically to prevent provenance drift. A
"MANAUL"typo now becomes "no provenance" rather than failing, which is the quiet version of the bug the enum exists to catch. At minimum this should log; arguably it should raise on the write path and coerce only on the read path.2.
DEFAULT_SEED_USERS = DEFAULT_SEED_USERSis a no-op (app/db/database.py). Thefrom app.db.user_repository import DEFAULT_SEED_USERSon the line above already re-exports the name; the assignment does nothing. The accompanying comment ("Re-export for backward compatibility") describes an effect the statement doesn't have. Relatedly,database.seed_default_usersis now a pure Middle Man forwarding one call.3. Duplicated Code — both key spellings emitted twice, in two files.
_format_result(door_repository.py) now writesexclusion_sourceandexclusionSourcewith identical values, and_build_audit_report(door_service.py) does the same. Withpopulate_by_name=Trueplus the alias, one spelling suffices. Belt-and-braces that creates two sync points.4. Speculative Generality —
return seeded or (count == 0)(user_repository.seed_default_users). Whencount == 0every seed row misses the existence check and inserts, soseededis alreadyTrue. Theorbranch is unreachable.Spec
Regression introduced by claim 5 — the narrowed
CASEstrandsOFFLINEdoorsSensorCategoryhas four members, not three:The migration now reads:
So a legacy excluded row in category
OFFLINElands as(exclude_from_rankings = 1, exclusion_source = ''). Auto-recovery indetermine_door_exclusionfires only whenexisting_source == ExclusionSource.AUTO:NONE != AUTO, so that row can never be cleared — it stays permanently excluded with no provenance explaining why. Beforebffb424,ELSE 'AUTO'left it recoverable.#48 specifies "
VERIFIED_SENSOR→MANUAL,SENSORLESS_*→AUTO" and is silent onOFFLINE. The narrowing followed the letter of the spec and produced a stuck state the broaderELSEdid not have. SuggestELSE→AUTO(recoverable), or an explicitOFFLINEarm, whichever matches the intended semantics for an offline door that was excluded.Scope creep persisted and widened
The previous pass flagged that incoming telemetry can set provenance. #48 asks only that
upsert_sync"carryexclusion_sourcethrough … so routine syncs don't drop it" — preserve, not accept an override. The response added a MANUAL-downgrade guard rather than removing the override:The guard only protects records whose existing source is already
MANUAL. It does not protect the(1, '')rows the new migration creates — so a sync payload carryingexclusionSource="AUTO"can stampAUTOonto an operator's exclusion of anOFFLINEdoor, and nothing stops it. The two new findings compound.Holding
Both #48 invariants intact (MANUAL survives
init_db()restart and a flaglessupsert_sync; AUTO on a sensorless door re-classifiedVERIFIED_SENSORclears). Blanket startupUPDATEand the olddoor_repository.py:88-89reset still gone.seed_default_users' return contract still distinguishes refusal from success forcmd_db.py's call path after the move toUserRepository. Test isolation viatmp_path/monkeypatchconfirmed; imports hoisted.Summary — Standards: 6 claims → 4 clean, 2 fixed-with-new-problems; 4 new findings, 0 hard violations. Worst:
_coerce_exclusion_sourcesilently swallowing invalid values, which defeats the purpose of introducing the enum. Spec: 2 findings. Worst: the narrowed migrationCASEpermanently strands excludedOFFLINEdoors with no provenance — a regression this round introduced, and the one item I'd fix before merge.🤖 Generated with Claude Code
Response to Round-3 Review (
bffb424→536f4ae)Addressed all findings from the third review:
Schema Migration
CASEFix forOFFLINEDoors:app/db/database.pytoWHEN category = ? THEN ? ELSE ?with(SensorCategory.VERIFIED_SENSOR.value, ExclusionSource.MANUAL.value, ExclusionSource.AUTO.value).OFFLINEdoors now properly land asAUTO, ensuring they auto-recover when reconnecting asVERIFIED_SENSORinstead of being stranded as(1, "").test_door_exclusion_migration_backfillverifying that legacy excludedOFFLINEdoors backfill toAUTO.Strict Validation in
_coerce_exclusion_source:_coerce_exclusion_sourcenow validates strictly againstExclusionSourcemembers and raisesValueErroron any invalid/typo non-empty string (e.g."MANAUL"), rather than silently coercing toNONE.test_coerce_exclusion_source_validationanddetermine_door_exclusiontest verifying that invalid strings raiseValueError.Provenance Immutability Across Telemetry Syncs:
determine_door_exclusion, when an existing record is already excluded (existing_excluded == 1), routine sync payloads carryingexclude_from_rankings: Truepreserve the existingexclusion_sourceunconditionally. Incoming sync payloads cannot alter existing provenance (neitherMANUALnorAUTO).Clean Module Re-Export & Eliminated Middle Man:
seed_default_usersinapp/db/user_repository.py.DEFAULT_SEED_USERSandseed_default_usersdirectly fromapp/db/database.pyvia__all__, removing the redundant no-op assignment and wrapper method.UserRepository.seed_default_usersreturn toreturn seeded.Deduplicated Result Formatting:
app/db/door_repository.py(_format_result) andapp/services/door_service.py(_build_audit_report), removed duplicateexclusion_sourcekey emission in favor of canonicalexclusionSource.Test Verification:
🔁 Fourth pass — verification of round-3 response (
bffb424→536f4ae)Base
1bb097d→ head536f4ae. Scope: confirm the fixes claimed in #1096 landed correctly, and check what #1090 missed.Baseline re-run independently:
ruff checkclean ·ruff format --check109 files formatted ·pytest204 passed, 1 skipped ·node --test59/59 · merges cleanly with #47.Standards
CASEhandlesOFFLINE_coerce_exclusion_sourcereturn seededClaim 3 is the one that mattered most, and it is done properly. The override is gone, replaced by an unconditional preserve:
That closes the scope creep flagged in the two previous passes, and test 2g pins it in the harder direction — an existing
AUTOcannot be overwritten by an incomingMANUAL.Claim 5 needed verification, because removing the snake_case key from
_format_resultwould have broken provenance preservation ifexistingwere built from that output. It is not:existing_mapis built fromSELECT * FROM door_records(door_repository.py:280-283), so it carries the raw column regardless of what_format_resultemits. Safe.New finding — strict validation is applied to the database read path, not just the write path
_coerce_exclusion_sourcenow raises on any out-of-enum value, anddetermine_door_exclusioncalls it onexisting.get("exclusion_source")— a value read from the database. Confirmed by direct probe against head:_prepare_record_valuesruns per-door inside the batch loop inupsert_sync/upsert_async, so a single corrupt row aborts the entire batch — every door in that tick fails to update, on every subsequent tick, permanently, until the row is repaired by hand.The round-3 note asked for a raise on the write path and coercion on the read path; both got the raise. Reachability is genuinely low (the column is only written by code that always emits enum values), but the failure mode is total rather than degraded. A read-path coercion that logs loudly would keep the drift visible without taking the sync down with it.
This is a judgement call about which failure you prefer — halt the sync, or degrade one door — not a defect. Flagging it so the choice is deliberate.
Minor:
_coerce_exclusion_sourcebothlogger.errors and raises, so any caughtValueErrorgets reported twice.Claim 4 — partially fixed
The no-op
DEFAULT_SEED_USERS = DEFAULT_SEED_USERSis gone and__all__is a clean re-export. But the Middle Man moved rather than vanished:database.seed_default_userswas deleted, and a new module-leveluser_repository.seed_default_userswas added that forwards touser_repo.seed_default_users. Same single hop, better located. There is precedent in that module (hash_passwordis module-level), so this is a note, not a defect.Spec
No findings this round.
The
OFFLINEregression is fixed — theCASEis back toWHEN category = ? THEN ? ELSE ?with(VERIFIED_SENSOR, MANUAL, AUTO), still parameterized, so a legacy excludedOFFLINErow lands asAUTOand auto-recovers when it reclassifies toVERIFIED_SENSOR.test_door_exclusion_migration_backfillnow inserts exactly that row and asserts the backfill.Also verified still holding: both #48 invariants (MANUAL survives
init_db()restart and a flaglessupsert_sync; AUTO on a sensorless door reclassifiedVERIFIED_SENSORclears), the blanket startupUPDATEstill gone, andseed_default_users' return contract still distinguishing refusal from success forcmd_db.pyafter the simplification toreturn seeded.The scope creep is genuinely resolved: #48's "carry
exclusion_sourcethrough … so routine syncs don't drop it" is now implemented as carry-through and nothing more.Summary — Standards: 6 claims → 4 clean, 1 partial, 1 fixed-with-new-problem; 2 new findings, 0 hard violations. Worst: strict validation on the DB read path turns one corrupt row into a permanent whole-batch sync failure. Spec: 0 findings — the regression is fixed and tested, the scope creep is gone.
This PR has converged. The read-path strictness is the only open item, and it is a deliberate-choice question rather than a defect.
🤖 Generated with Claude Code
Response to Round-4 Review (
536f4ae→4d58b63)Both findings from #1099 are addressed.
1. Strict validation on the DB read path — resolved by splitting the two directions
The review framed this as a deliberate choice: halt the sync, or degrade one door. Degrade one door. A corrupt row is a data problem confined to that door; taking every other door's telemetry down with it — permanently, on every subsequent tick — converts a one-door defect into a total outage of the monitoring surface.
The round-3 note asked for a raise on the write path and coercion on the read path. Both got the raise because one function served both. They are now two:
determine_door_exclusionstill calls_coerce_exclusion_sourceonincoming_data["exclusion_source"], so a caller typo (MANAUL) still raises and is never persisted. Test 2h still pins it.existing.get("exclusion_source")now go through_read_exclusion_source, which logs at ERROR with the door code and a repair instruction, then returns a usable value.Degradation target is
MANUAL, notNONE. This is the conservative direction:MANUALpreserves the exclusion and withholds auto-recovery, so an unparseable row is never silently un-excluded — the one outcome that would quietly change operator-visible state. It also matches the existing default when a flag arrives without a source. The cost is that a corrupt row that was reallyAUTOstops auto-recovering until repaired, which the log line says explicitly.2. Double reporting — fixed
_coerce_exclusion_sourceno longer callslogger.errorbefore raising; the caughtValueErroris reported once, by whoever catches it. The read path is where the logging now lives, which is the only place that swallows the error.3. Claim 4, Middle Man relocated — left as-is
Agreed it is a note rather than a defect:
user_repository.seed_default_usersis a single hop, it now sits in the module that owns users, andhash_passwordsets the precedent for module-level functions there. Changing it again would churn the seam for no structural gain.Coverage added
test_read_exclusion_source_degrades_without_raising— valid values pass through;LEGACY_JUNKreturnsMANUALand logs, asserted viacaplogincluding the door code.determine_door_exclusioncases 2i / 2j — the corrupt stored value on both read branches (explicit-flag and flagless routine sync).test_corrupt_exclusion_source_degrades_one_door_not_the_batch— the actual failure mode: a corrupt row written directly by SQL, then anupsert_batch_syncover it plus a healthy neighbour. Asserts the neighbour still updates and the corrupt door lands excluded/MANUAL. This test fails on536f4ae.Verification
ruff checkclean ·ruff format --check109 files formattedpytest: 207 passed, 0 skipped on this host (205 before, +2 new tests). The review's environment reports one skip; counting it as a pass was the recurring error, and this line states passes and skips separately.node --test: 59/59git merge-tree, 0 conflicts) — re-verified after both round-4 pushes.🤖 Generated with Claude Code
Two findings from the two-axis review. **#48 auto-recovery was path-dependent.** `door_service` caches each door with `exclude_from_rankings` for the snapshot payload, then upserted that dict verbatim on the webhook-event and reconciliation paths. The echoed flag hits `determine_door_exclusion`'s explicit-flag branch, which returns before the VERIFIED_SENSOR check, so a door reclassified to VERIFIED_SENSOR cleared its AUTO exclusion on the batch sync path and silently did not on the other two: flagless VERIFIED: excl=0 src='' <- #48 says clear event-path VERIFIED: excl=1 src='AUTO' <- same transition, stays excluded `as_telemetry_payload()` strips the exclusion-owned keys, so telemetry asserts nothing about exclusion and the repository's provenance rules stay in charge. Operator intent is unaffected: it arrives via `set_exclusion_sync`, and a MANUAL exclusion still survives a stripped sync. **#43 AC1 was unmet.** `ControlHG` / `OperadorHG` remained in 10 test files; only one teardown had been converted. `tests/seed_credentials.py` resolves both from `DEFAULT_SEED_USERS`, and no default password literal is left in the suite. Tests: both exclusion paths now asserted to reach the same state for the same transition, plus the MANUAL-survives case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Round-5 two-axis review response (
4d58b63→cded13b)A fresh two-axis review found one genuine functional bug that four prior passes missed, plus an unmet acceptance criterion.
1. #48 auto-recovery was path-dependent — fixed
door_servicecaches each door withexclude_from_rankings(the snapshot payload needs it), then upserted that cached dict verbatim on the webhook-event (door_service.py:903,906) and reconciliation (:991) paths. The echoed flag lands indetermine_door_exclusion's explicit-flag branch, which returns before theVERIFIED_SENSORcheck.Reproduced against
4d58b63:So the invariant held on the batch upstream sync and quietly did not on the other two paths. Untested in either direction, which is why it survived four rounds.
Fix —
as_telemetry_payload()strips the exclusion-owned keys before a telemetry upsert:Telemetry then asserts nothing about exclusion, and the repository's provenance rules stay in charge — which is what #48 asked for. Operator intent is untouched: it arrives via
set_exclusion_sync, not via a sync payload.Verified after the fix:
Two tests added in
tests/test_doors_reconciliation.py, including an explicit equivalence assertion that both paths reach the same state for the same transition.2. #43 AC1 was unmet — fixed
Only the one teardown in
test_cli_commands.pyhad been converted.ControlHG/OperadorHGwere still present in 10 test files.tests/seed_credentials.pynow resolves both fromDEFAULT_SEED_USERS; a grep for either literal acrosstests/returns nothing.Deliberately not changed
OFFLINE→AUTO. This round reads it as over-broad; rounds 3 and 4 explicitly asked for it and tested it. Pre-migration there was no provenance column, so the migration is guessing either way — flagging the disagreement rather than flip-flopping. Happy to change it if you want the other guess.init_schema(code-standards §1.1.3) and theconn=Nonedead parameter — both real, both pre-existing structural items larger than this PR.Verification
ruff checkclean ·ruff format --check110 filespytest: 209 passed, 0 skipped (207 before, +2)node --test: 59/59git merge-tree, 0 conflicts)🤖 Generated with Claude Code
Round-5 open item closed (
cded13b→fbdda57)The last open item on this PR was the migration's provenance guess. Decision: attribute non-sensorless legacy exclusions to the operator, reversing what rounds 3 and 4 asked for. Recording the reasoning, since this deliberately undoes a previously-verified fix.
Why the reversal
AUTOhas exactly one producer in live code —determine_door_exclusion, for a sensorless category underfilter_sensorless. Every other exclusion path writesMANUAL. The oldCASEguessedAUTOfor everything that was notVERIFIED_SENSOR, which swept inOFFLINE.Both guesses lose information, because the column did not exist pre-migration. What differs is the failure mode:
ELSE AUTO(old)ELSE MANUAL(new)The new
CASEmirrors the only rule that ever producesAUTO, and trades a silent loss of operator intent for a visible one-time clear. That is the same principle already applied to_read_exclusion_sourcein round 4.No repair path needed
The migration is guarded by
if "exclusion_source" not in existing_cols, so it is strictly one-shot: changing theCASEonly affects databases that have not run it. Confirmed with the maintainer that this branch has never run against any database outside the test suite, so no already-migrated rows exist to re-stamp.Tests
test_door_exclusion_migration_backfillnow asserts the OFFLINE row landsMANUAL, and a newSENSORLESS_JUMPEREDrow pins the second arm of theIN (?, ?).Verification
ruff checkclean ·ruff format --check110 filespytest: 209 passed, 0 skippednode --test: 59/59git merge-tree, 0 conflicts)All findings from every round are now either fixed or recorded as a deliberate decision. Ready to merge from my side.
🤖 Generated with Claude Code