span-panel-api 3.1.1 and schema-0 1.1.1: release-review fixes - #169
Merged
Conversation
A pydantic-v2 field-level 422 reports the failure by pointing `loc` at the field and putting the rejected value under `input`, where it is a bare string. The key-based walk had nothing to judge it by, so `_redact` returned that body byte-identical and the passphrase went into the DEBUG log. A scalar in an error object whose `loc` names a credential field is now redacted on the strength of that `loc`; a dict or list under the same key is still walked, so the surrounding diagnostic survives. And `register_v2` now passes the passphrase it sent into the redaction, which closes the case structure cannot reach at all -- a panel quoting the credential back in free prose.
Seven of the eight caught `httpx.ConnectError` and `httpx.TimeoutException` and nothing else, so a `ReadError` from a panel resetting its listener mid-response, or a `RemoteProtocolError` from a proxy closing without answering, came back as a raw httpx exception -- not a `SpanPanelError`, and so caught by none of the retry clauses built on that contract. The bodies were unguarded the same way: a 200 missing a field raised `KeyError` and one that was not JSON raised `JSONDecodeError`. `_request` performs the call and translates the whole `TransportError` family; the `_Reply` it returns decodes the body and names the field the panel left out. Status classification stays with each caller, because it is genuinely per-endpoint -- 412 means "no passphrase is set" on one path and nothing anywhere else. `get_homie_schema` keeps its retryable reading of an unparseable body by asking for `SpanPanelServerError`, and the detector now catches this library's own two classes instead of a hand-listed tuple of httpx ones that had let `ReadError` out of a function documented to return a result.
The MQTT bridge already says once per bridge that an unpinned trust anchor is fetched over plaintext HTTP and whatever answers is trusted. The REST side said nothing, though registration is the request that carries the panel passphrase up and brings the broker password back -- so anything on the path reads both, and an operator had no way to tell the encryption was off. Warned by the two calls that bootstrap a client, not from inside `_request`: one line per client is a line somebody reads, one per request is a line somebody filters out. `register_v2` warns for itself and the factory warns only on the branch that never reaches it, so a caller never hears it twice. Plaintext remains the default -- requiring a pin would break every install on upgrade -- but it is now a default the log names.
`/api/v2/status` was parsed in two places -- the detector's, deciding whether the panel speaks v2 at all, and `get_v2_status`'s, reading the same answer for a caller that already knows it does -- and the two had drifted. Only the detector read `proximityProven`, so the same panel reported "proximity unknown" or "proximity proven" depending on which call had asked. `V2StatusInfo.from_status_payload` is now the only reader, so the field a caller gets no longer depends on the route it took. Absence still reads as `None` rather than false, because firmware below 202609 does not report proximity at all and that is not the same fact as not proven.
`denormalize_circuit_id` restored dashes to a dashless UUID, and nothing asked it to. The adapter normalises ids on the way in for entity stability and never converts them back -- no parser, no snapshot, no topic builder and no consumer called it, and the only two references anywhere were the tests written for it. It is not part of the `SchemaAdapter` protocol and was never re-exported from the package, so nothing outside could reach it either.
The adapter's floor on the bootstrap stays at 3.1.0 and the bootstrap's extras stay at 1.1.0: nothing in either release asks anything new of the other, so neither pairing that resolved before stops resolving now.
`detect_api_version` is documented to return a result, and every caller reads one rather than guarding the call. Since the body validation went in, a 200 carrying something that is not a status object raised out of it instead -- and the case that produces that is a proxy in front of a v1 panel answering its own HTML error page under a 200, which is a panel the caller was only asking about. Reported as a failed probe rather than a v1 verdict: no answer was read, so the honest report is that the question is still open. `probe_failed` now documents that reading, which covers both a request that did not complete and a response that could not be understood.
The warning was raised by two call sites, which meant every new call site had to remember to raise it -- and `regenerate_passphrase` did not. That is the call a consumer reaches for when reauthenticating: it sends a bearer token up and brings the new broker password back over the same plaintext transport registration uses, and it said nothing at all. Moved into `_request`, so there is one mechanism and nothing to remember, and the changelog's "once per client" is now literally true of every call rather than of two of them. Scoped to the panel rather than the client object, which looks tighter and is worse: the CA download runs on every MQTT reconnect and builds a fresh client each time, so that key would produce a warning per reconnect -- exactly what the bridge's own once-per-bridge warning exists to avoid.
…ored in `_create_ssl_context` is annotated to return an `SSLContext` and its last statement returned `_ssl_cache.context`, which is typed `SSLContext | None`. The assignment on the line above does not make the read that follows safe: the field is shared mutable state, so another task may clear or replace it in between, and a strict checker is right to object. Handing back the local the executor just produced says what the function actually means and needs no narrowing to be true.
Two loose ends from review. `_redact` reads as three independent rules, so the residual case -- a scalar the walk cannot place -- looks unhandled when read on its own; it is covered because the sole call site always passes the passphrase it sent, and the docstring now says so rather than leaving the reader to find the call site. And the redaction tests carried an import of `unittest.mock.patch` that nothing used, which the test tree's blanket lint exclusion had been hiding.
…th the other module state The comment claimed the once-per-host set had the same scope as the MQTT bridge's unpinned-CA warning. It does not: the bridge's flag is per instance and repeats on a config-entry reload; this set is process-wide. The per-host choice stands -- the CA download builds a fresh client per reconnect -- but the comment now says what the scope is rather than what it resembles. The test reset moves to conftest beside the SSL-cache reset, so any test that drives a plaintext request starts unwarned, not only the ones in the file that happens to assert on it.
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
1 participant
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.
Follow-up to the 3.1.0 release review. No API change.
detail[].inputwith the key only inloc) no longer reaches the debug log; redaction is by key, by siblingloc, and by the value the caller sent._request()helper carries every bootstrap REST call: the wholehttpx.TransportErrorfamily becomesSpanPanelConnectionError, and a 200 with a non-JSON, non-object or key-missing body becomesSpanPanelAPIErrorinstead of a rawJSONDecodeError/KeyError.regenerate_passphraseis covered as well as registration./api/v2/status, shared by detection andget_v2_status; detection answersprobe_failedon an unreadable status body rather than raising.denormalize_circuit_idremoved.mypy --strict clean (41 files); ruff clean; 1159 passed, 13 skipped.