Fix evaluation findings: proto3 absence detection, auth retry, stream resilience#12
Merged
Conversation
Truthiness and getattr-with-default absence checks never fire on real protobuf messages (unset sub-messages return truthy default instances), so sparse stream diffs zeroed room state, IDU controls, sensor readings, and controller temperatures on merge. Parse absence via HasField-based helpers, and preserve identity/relationship fields, controller temperatures, ODU performance data, and QSM led color in the SystemSnapshot.apply_* merges. Add real-proto round-trip merge tests that reproduce the notifier stream's serialize/parse path.
…lation In grpc.aio, awaiting the interceptor continuation only returns the call object — AioRpcError surfaces when the call itself is awaited, so the token-refresh retry was dead code and clients broke permanently on token expiry. Await the call inside the interceptor and retry once on UNAUTHENTICATED. Also: never duplicate authorization metadata already present on a call, pass CancelledError/KeyboardInterrupt through grpc_call untranslated, map NOT_FOUND to QuiltNotFoundError, route all hds/user RPCs through grpc_call so UNAVAILABLE surfaces as QuiltConnectionError everywhere, echo raw comfort-setting fan wire values (absent vs AUTO), and fetch-and-echo the current phone number when update_current_user is called without one.
- Reset reconnect budget and back-off after a healthy connection, so routine server-side stream recycling no longer escalates every reconnect to 60s or permanently kills the stream after N lifetime disconnects - Catch non-gRPC failures (parse crashes, metadata provider errors, channel usage errors) in the reconnect loop: reconnect while budget remains, then surface via on_error instead of silently dying - Skip malformed events instead of letting one bad payload kill the stream - Reconnect immediately after a successful token refresh - Add jitter to back-off to avoid fleet-wide reconnect stampedes - Add on_connected callback so consumers can re-snapshot after gaps - on_* registrations return unsubscribe callables - subscribe/unsubscribe before start no longer queue duplicate requests - stop() logs pre-existing task errors instead of masking the caller's exception; derive wire-scan field numbers from the proto descriptor
…ards - authenticate() no longer returns a locally-unexpired token that the server just rejected: transport/stream UNAUTHENTICATED contexts force a Cognito refresh - Persist rotated refresh tokens instead of silently discarding them - Measure token expiry from receipt time, not from before the OTP prompt - QuiltClient.refresh_token() is single-flight: concurrent 401s trigger one refresh instead of a Cognito stampede with racing store writes - get_snapshot() cold-cache fetch is single-flight - get_system_id(home=...) with a non-default home no longer poisons the cached default system (and matches the configured home case-insensitively) - close() stops tracked NotifierStreams before closing the channel - _resolve_snapshot_item and home resolution raise QuiltNotFoundError - tokens.py: parenthesize except (works on 3.13 parsers/tools too) and fix the refresh-callback signature cache to key bound methods by __func__ so it actually hits
TUI: register stream on_error with visible disconnect indicator and automatic recovery (re-snapshot + restart with backoff); OTP modal so first-time users can log in; boot failures show an error screen instead of a stuck spinner; app-owned snapshot shared by all screens (stacked screens no longer render from a stale object after auto-refresh); dashboard cursor preserved across refresh and rows re-rendered on resume; app-level °C/°F preference; setpoint clamping with named bounds; LED toggle-on restores stored/default brightness; 0.0 readings render as values instead of '--'; dead code removed and duplicated schedule/ODU logic extracted. CLI: energy --period validates choices; --heat/--cool enforce range; 'quilt set' with no options errors instead of a no-op RPC; FALLBACK_* modes rejected; OTP prompt no longer blocks the event loop. Store: corrupt tokens.json is quarantined to a timestamped backup and recovered (was a permanent hard failure), atomic writes fsync before replace, advisory flock around read-modify-write, config dir 0700.
- README Quick Start now merges stream diffs via snapshot.apply_space instead of demonstrating the documented misuse; client.stream() docstring examples likewise - Re-export NotifierStream, StreamEvent, and token protocol types at the package top level so consumers don't import from quilt_hp.services - Update reference docs: real OutdoorUnit/Controller dataclasses (the documented OutdoorUnitState/ControllerState never existed), nullable controller temperatures, apply_software_update_info, on_connected and unsubscribe callbacks, reconnect back-off semantics, rotated refresh token persistence - Fix contradictory proto comments: led_animation/led_state field-number note and louver_fixed_position fraction-vs-degrees
The .pyi stubs embed proto comments as docstrings, so the comment-only corrections in quilt_hds.proto require regenerated stubs to keep the proto-sync CI check green.
There was a problem hiding this comment.
Pull request overview
This PR hardens the library’s core “snapshot + sparse stream diffs” architecture by fixing proto3 presence detection (preventing zeroed/empty state on merges), making auth refresh/retry paths actually execute under grpc.aio, and improving stream reconnect resilience. It also extends test coverage to include real generated protos and updates CLI/TUI behavior and documentation to match the correct integration pattern.
Changes:
- Switch model parsing/merge logic to explicit proto3 presence detection and preserve identity/relationship/control fields across sparse diffs.
- Fix gRPC auth interceptor retry semantics for
grpc.aio(await call objects), and dedupe authorization metadata. - Improve NotifierStream reconnect/backoff behavior, callback API (unsubscribe handles +
on_connected), plus CLI/TUI and token-store robustness updates.
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_tui_bindings.py | Adds TUI-focused regression tests for bindings and helpers. |
| tests/test_transport_interceptor_extra.py | Updates interceptor tests to model real grpc.aio call semantics + metadata behavior. |
| tests/test_tokens.py | Updates token-store tests for corrupt JSON recovery behavior. |
| tests/test_streaming.py | Adds/adjusts stream subscription tests (no duplicate pre-start requests). |
| tests/test_streaming_reconnect_dispatch_extra.py | Expands reconnect/dispatch tests (jitter, budget reset, non-gRPC errors, on_connected). |
| tests/test_models_real_proto_merge.py | New: end-to-end sparse-diff merge tests using real generated protos and wire roundtrips. |
| tests/test_models_from_proto.py | Adjusts model parsing tests to simulate absent sub-messages. |
| tests/test_hds_service_branches.py | Updates HDS service test data for raw fan speed wire fields. |
| tests/test_client_service_error_paths.py | Updates UserService error-path test with new phone handling. |
| tests/test_client_cache.py | Adds client cache/single-flight tests (system-id caching, snapshot fetch, close stops streams). |
| tests/test_cli_surfaces_extra.py | Adds CLI validation tests (set command options, modes, setpoint bounds, energy period). |
| tests/test_auth.py | Adds auth tests for forced refresh after server rejection + rotated refresh token persistence. |
| tests/test_auth_store_settings_edges.py | Adds FileStore recovery tests and helper for corrupt backup listing. |
| src/quilt_hp/transport.py | Fixes interceptor retry by awaiting call objects, and avoids duplicating auth metadata. |
| src/quilt_hp/tokens.py | Improves refresh-callback signature caching for bound methods. |
| src/quilt_hp/services/user.py | Routes UserService calls through unified grpc_call; preserves phone_number semantics. |
| src/quilt_hp/services/streaming.py | Improves reconnect/backoff/jitter, error handling, callback API (unsubscribe + on_connected), and safer wire scanning. |
| src/quilt_hp/services/hds.py | Routes HDS RPCs through grpc_call and preserves fan-speed wire values on comfort updates. |
| src/quilt_hp/services/init.py | Adds NOT_FOUND translation and ensures cancellation-like exceptions aren’t translated/swallowed. |
| src/quilt_hp/models/system.py | Makes apply_* merges preserve identity/relationships/telemetry across sparse diffs; adds apply_software_update_info. |
| src/quilt_hp/models/space.py | Switches Space parsing to presence-gated sub-messages; adjusts display behavior for DRY mode. |
| src/quilt_hp/models/sensor.py | Presence-gated parsing for state/controls/attrs to avoid zeroing readings on diffs. |
| src/quilt_hp/models/qsm.py | Presence-gated parsing for QSM controls/state/wifi to preserve sensors across diffs. |
| src/quilt_hp/models/outdoor_unit.py | Presence-gated ODU parsing; models hvac_state as HVACState and preserves telemetry. |
| src/quilt_hp/models/indoor_unit.py | Presence-gated IDU parsing across multiple sub-messages; uses shared timestamp helper. |
| src/quilt_hp/models/controller.py | Presence-gated controller state/wifi parsing; makes temperatures optional for sparse diffs. |
| src/quilt_hp/models/comfort.py | Stores raw fan speed wire values to avoid “absent vs AUTO” ambiguity on updates. |
| src/quilt_hp/models/_helpers.py | Adds shared proto presence helpers and timestamp conversion helper. |
| src/quilt_hp/client.py | Adds auth/snapshot single-flight locks, better system-id caching semantics, stream tracking for close(). |
| src/quilt_hp/cli/tui.py | Improves TUI UX/resilience: OTP modal, boot error screen, snapshot ownership, schedule pause patching, reconnect recovery. |
| src/quilt_hp/cli/store.py | Adds corruption recovery, fsync, flock-based locking, and safer token store IO behavior. |
| src/quilt_hp/cli/main.py | Adds stricter CLI validation for setpoints/modes and typed energy period handling. |
| src/quilt_hp/cli/constants.py | New: shared CLI/TUI constants and setpoint clamping. |
| src/quilt_hp/auth.py | Forces refresh when server rejected token; persists rotated refresh tokens; measures expiry from receipt time. |
| src/quilt_hp/_proto/quilt_hds_pb2.pyi | Updates regenerated stub comments to match corrected proto commentary. |
| src/quilt_hp/_paths.py | Creates config dir with user-only permissions (0o700). |
| src/quilt_hp/init.py | Re-exports stream/token/auth types at top-level API. |
| README.md | Updates streaming example to merge sparse diffs into a snapshot. |
| proto/cleaned/quilt_hds.proto | Fixes proto comments (IndoorUnitControls field ordering and louver semantics). |
| docs/reference/token-management.md | Documents forced refresh and refresh-token rotation persistence on server-rejected tokens. |
| docs/reference/models.md | Updates streaming API docs (unsubscribe handles, on_connected) and model reference updates. |
| docs/reference/hds-entities.md | Updates controller temperature field types for sparse diffs. |
| docs/reference/client.md | Documents close() stopping tracked streams and clarifies consecutive reconnect semantics. |
| docs/explanation/streaming-protocol.md | Documents jitter, budget reset after healthy connections, prompt reconnect after refresh, and on_connected. |
- intercept_unary_stream: wait_for_connection on the retried call and raise QuiltAuthError when the retry is still UNAUTHENTICATED, matching the unary path instead of deferring the failure to first iteration - streaming: dispatch callbacks over a snapshot of the registration lists so a callback can unsubscribe itself (or others) mid-dispatch - store: add a uuid suffix to corruption backup names — the 1s timestamp resolution could collide and leave the corrupt file in place
eman
force-pushed
the
bugfix/evaluation-fixes
branch
from
July 2, 2026 23:19
d20756f to
541694a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes all findings from the full architecture/code/performance/bug-hunt evaluation.
Critical
SystemSnapshot.apply_*preserves identity/relationship fields, controller temperatures, ODU telemetry, and QSM LED state. New test suite (tests/test_models_real_proto_merge.py) reproduces the notifier stream's serialize/parse path with real generated protos.High
on_errorinstead of dying silently; malformed events are skipped; jitter added; prompt reconnect after successful token refresh.authenticate()no longer returns a locally-unexpired token the server just rejected; rotated Cognito refresh tokens are persisted; expiry measured from token receipt.grpc_callpassesCancelledErrorthrough untranslated (asyncio.timeout/cancellation semantics restored).Medium / Low
grpc_call;UNAVAILABLE/DEADLINE_EXCEEDED→QuiltConnectionErrorandNOT_FOUND→QuiltNotFoundErroreverywhere.get_system_id(home=...)no longer poisons the default-system cache;close()stops tracked streams; duplicate authorization metadata eliminated.on_connectedcallback, unsubscribe handles from allon_*registrations,apply_software_update_info, descriptor-derived wire-scan field numbers.Intentionally unchanged: Python >= 3.14 floor and boto3 dependency (HA 2026.3+ runs Python 3.14).
Validation
ruff check + format, mypy (44 files), full pytest (89% coverage), check_docs_nav.py, mkdocs build --strict — all green.