breaking: screaming-goblin - #143
Draft
JPHutchins wants to merge 8 commits into
Draft
JPHutchins wants to merge 8 commits into
JPHutchins wants to merge 8 commits into
Conversation
smp's `screaming-goblin` replaces the flattened pydantic message model with composition -- a message is a `Frame[T] = (Header, Data[T])` on `msgspec.Struct` -- and bakes each request's `_Response`/`_ErrorV1`/`_ErrorV2` binding into the request class itself. This ports smpclient onto it. Part of intercreate/smpmgr#103; closes #124. Net -872 lines. ## The request layer is gone (#124) `smpclient/requests/**` was a parallel class layer that existed only to attach `_Response`/`_ErrorV1`/`_ErrorV2` to each smp request. smp does that itself now, so the layer is deleted outright rather than deprecated: `screaming-goblin` is the breaking release. Callers move to smp's own names, which differ -- `GroupCountRequest` rather than `CountSupportedGroups`, `ImageStatesReadRequest` rather than `ImageStatesRead` -- across 25 files here plus smpmgr. `SMPRequest` now comes from `smp`. The four narrowers stay in `smpclient.generics`: smp deliberately does not ship them, as its own typing test says outright ("without the `smp` library having to ship them"). Their signatures are unchanged. smp's non-generic `TypeIs` narrowers were tried first and rejected: they erode `TRep` to `ReadResponse | WriteResponse` for a *generic* caller, which breaks the `ensure_request` helper both `examples/*/ upgrade.py` are built around. smp's typing test only exercises concrete request types, so it does not cover that case. The cost of keeping the generic form is 5 `reportInvalidTypeVarUse` warnings, which do not gate (#134). ## request() The frame is now built once, because it carries the sequence the response must echo: request_frame = request.to_frame() ... send bytes(request_frame) ... if header.sequence != request_frame.header.sequence: raise SMPBadSequence `loads()` returns a `Frame`, so the client returns `.data`. The decode chain catches `msgspec.DecodeError`, not `ValidationError`. Both are reachable and the wider one is deliberate: a *schema* mismatch raises `ValidationError`, but a payload that is not decodable CBOR at all -- truncated, or empty -- raises a bare `DecodeError`. Catching only `ValidationError` would let a truncated response escape as a raw msgspec traceback instead of the `SMPValidationException` carrying the header and hexdump. Under smp 4.x a raw cbor2 error escaped uncaught, so this is also a small improvement. `SMPMalformed` and `SMPMismatchedGroupId` are *not* caught. They fail all three candidate types identically, which makes them transport errors rather than "this frame matched no schema", and swallowing them into the try-chain would report a group mismatch as three parse failures. Both cases are now covered by tests. msgspec reports a decode failure as a single message rather than a structured list, so `_format_validation_error` is gone and `_validation_failure` prints the message; the header and hexdump it reports are unchanged. ## Size math `bytes(Data)` is the CBOR payload alone -- the old `bytes(message)` included the 8-byte header -- so `get_max_cbor_and_data_size` subtracts `Header.SIZE` explicitly and derives the CBOR size from `len(bytes(request))` rather than the header's `length` field. `_maximize_upload_packet` collapses to `msgspec.structs.replace(request, data=...)`: `to_frame()` computes `length` from the actual payload, so there is no header to build and no field-carrying to do. `_ic_maximize_packet` was a hand-rolled copy of that logic, needed only because pydantic made field-carrying manual; it is deleted and `ICUploadClient` calls the generic one, which required adding the Intercreate request to `TUploadRequest`. The byte-exact expectations in `test_maximize_upload_packet_fills_decoded_buffer` were left untouched and pass: the maximizer still fills `max_unencoded_size` exactly for buf_size 384/512/1024/2048. ## Tests `SMPMockTransport` now echoes the request's sequence back on the response, the way a real server does. `request()` draws the sequence from smp's counter when it frames, so a test cannot know it in advance -- and with the echo, the 22 hand-built `smphdr.Header(...)` blocks and all the `(h.sequence + 2) % 0xFF` bookkeeping simply disappear. A `sequence_offset` fakes a server answering out of order, for the `SMPBadSequence` case. `ErrorV1`/`ErrorV2` declare no `_OP` or `_COMMAND_ID`, so `to_frame()` cannot synthesize their header and a test that needs their bytes builds it by hand -- the same thing smp's own `test_error.py` does. That is what `error_bytes()` is. `tests/test_requests.py` tested the deleted layer and is deleted with it. ## Docs Deleting the request layer removes the only reason `docs/_generate_requests_docstrings.py` existed -- it back-filled inherited docstrings onto those subclasses. The script, its step in both `release.yaml` and `test-docs.yaml`, the seven per-group pages and their nav entries all go. `docs/requests.md` stays, because the narrowers it documents stay, and `docs/user/intercreate.md` now points at `smpclient.extensions.intercreate`, which is genuinely smpclient's. ## Dependency `smp` is pinned to the `screaming-goblin` branch by direct reference (hatchling needs `allow-direct-references` for that), because smp is not released with the break. Both are marked TODO to undo at release. `msgspec` is now declared: this module imports it directly for `DecodeError` and `structs.replace`, and relying on it arriving through smp is the same undeclared-transitive mistake #133 just fixed for pydantic. `typing_extensions` is now declared too. It is imported at runtime by twelve modules -- `generics` needs `TypeIs`, and the transports need `override` and `assert_never` -- but it was never declared: it arrived transitively via pydantic, via smp 4.x. Dropping pydantic from the tree took it with it, which the `transport-extras` CI job caught (`uv pip install .` resolves only declared dependencies, unlike the lockfile the other jobs use). That is the third undeclared-transitive dependency in this effort, after pydantic and msgspec. `smpclient/__init__.py` no longer imports pydantic, so with #133 the dependency is gone from `src/` entirely. ## Verification `camas matrix` green on Python 3.10-3.14. The integration suite passes 229/229 against real Zephyr servers -- serial, raw serial, COBS raw, UDP, and MCUboot serial recovery -- which is what establishes that the msgspec encoder puts the same bytes on the wire. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s links Three of the review items from #137. ## `smpclient.generics` is gone The four narrowers and the `TRep`/`TEr1`/`TEr2` TypeVars move into `smpclient/__init__.py`, beside the `request()` whose return type they narrow. The module was 64 lines of which the `SMPRequest` Protocol -- the only part that justified a separate module -- had already moved to smp. ## Static exhaustiveness tests `tests/test_generics_typing.py` drives the real `SMPClient.request` and asserts the narrowing for every group, verified under mypy *and* pyright. `assert_type` pins the exact narrowed type rather than merely asserting it is a response, so a binding that silently widened would fail; `assert_never` closes each union. That widening is not hypothetical, and this file is what proves the choice made in the parent commit: with smp's non-generic `TypeIs` narrowers, `assert_type(response, EchoWriteResponse)` fails because `success()` yields `ReadResponse | WriteResponse` instead of the request's own response type. The file is never executed, so it is omitted from coverage -- there is no runtime behaviour in it to measure. ## docs/requests.md Now carries the per-group links to smp's documentation, replacing the seven deleted pages that used to hold them, plus the narrowing example and the helpers themselves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Corrects a claim made in the two commits before this one. Those messages said smp's non-generic `TypeIs` narrowers "erode `TRep` for a generic caller", breaking the `ensure_request` helper in `examples/*/upgrade.py`, and that `tests/test_generics_typing.py` was the regression test proving it. That is false, and it is why the generic signatures were kept. The pyright error that produced the claim was a cascade from an unrelated broken import: at that moment the examples still did `from smpclient.generics import SMPRequest`, which had just moved to smp, so `SMPRequest` was an unknown symbol and `SMPRequest[TRep, TEr1, TEr2]` degraded to Unknown -- which is what made the narrowed value unassignable to `TRep`. Fixing the import fixed it; the narrowers were never involved. Verified by swapping the two forms in place and running the whole gate against each. Both are clean under mypy and pyright, `assert_type(response, EchoWriteResponse)` holds either way in `tests/test_generics_typing.py`, and the generic `ensure_request` helper type-checks under both. So take smp's form, which is the better one: the repository now has **zero** pyright warnings, down from five `reportInvalidTypeVarUse`. Those five were the reason #134 could not simply turn on `--warnings`; that is now unblocked, and the narrowers match the reference implementation in smp's own typing test. `TRep`/`TEr1`/`TEr2` stay -- `request()` still needs them for its signature -- they are just no longer threaded through the narrowers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resync onto smp `screaming-goblin` at b9003299, which closed JPHutchins/smp#70: `SMPRequest.to_frame()` now declares `sequence`/`version`/`flags`, the module-global `itertools.count()` in `smp/message.py` is gone, and `sequence` is required and typed `u8`. `SMPClient` therefore owns one counter per connection instead of every client in the process sharing one. Sequences are now monotonic per client rather than interleaved with whatever else is running, and the 8-bit space is no longer consumed N times faster with N clients. The narrowing follows smp#71's migration note. `u8` is a `Literal[0..255]` alias from types-bits with no runtime constructor, so masking alone does not satisfy either checker -- `next(counter) % 0x100` is still an `int` to them. It is narrowed once, at the boundary: def _next_sequence(self) -> "u8": return cast("u8", next(self._counter) % 0x100) That is the only cast, and it buys the header's 8-bit field being enforced by the type checker rather than by a `struct.error` at pack time. Test and example call sites pass a literal, which needs no narrowing -- and reads better than the implicit global it replaces. `types_bits` is imported only under `TYPE_CHECKING`; it arrives as a declared dependency of smp, so nothing new is declared here. `camas matrix` green on 3.10-3.14; integration 229/229. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ndency Both review comments on #137. ## The sequence space is a constructor parameter SMPClient(transport, address, sequence=iter((7, 9))) defaulting to `wrapping_sequence()`, the 0x00-0xFF space the client used to hold privately. A test can now pin exactly what goes on the wire instead of inferring it, and resetting or sharing a sequence space becomes a caller's decision rather than something to add later. `_next_sequence()` is gone -- `request()` just takes `next(self._sequence)`. Covered by `test_injected_sequence`, which asserts the injected values reach the header, and `test_wrapping_sequence`, which pins the default's range and wrap. ## `u8`, not `"u8"` The quotes were there because `types_bits` was imported under `TYPE_CHECKING`. Importing it at runtime instead removes them, but breaks a clean install: File ".../smpclient/__init__.py", line 56, in <module> from types_bits import u8 File ".../types_bits/__init__.py", line 20, in __getattr__ from annotated_types import Ge, Le ModuleNotFoundError: No module named 'annotated_types' `types-bits` declares `annotated-types` only under an `rt` extra, and says so itself: "`__init__.pyi` shadows this module for type checkers; this is the runtime tier." It is a typing-only package by design, so making every smpclient install carry `annotated_types` to unquote an annotation is the wrong trade. Instead the name is given a runtime value only checkers ignore: if TYPE_CHECKING: from types_bits import u8 else: u8 = int Every use is now bare -- the annotations and the one `cast` -- with no runtime dependency and no new declaration. Verified against a clean `uv pip install .`, which is the check that caught the problem. `camas matrix` green on 3.10-3.14; integration 229/229. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
breaking: port to smp screaming-goblin (Frame[T] on msgspec)
#137 bound `u8 = int` in an `else` branch of the `TYPE_CHECKING` import, so the checkers saw `types_bits.u8` (`Literal[0..255]`) while the interpreter saw `int`: an invisible divergence. The branch existed only because two expressions evaluated `u8` at runtime: the `sequence: Iterator[u8] | None` annotation on `SMPClient.__init__` (evaluated eagerly on 3.10-3.13) and the first argument of `cast()` in `wrapping_sequence()`. With `from __future__ import annotations`, annotations are stored as strings and never evaluated. `cast()`'s first argument is an ordinary expression, not an annotation, so it is quoted. Nothing binds `u8` at runtime any more, so the `else` branch and its explanatory comment go together. Why not declare `types-bits[rt]` instead: smp itself declares `types-bits<0.3,>=0.2` without the `rt` extra, so the library that defines `u8` treats it as typing-only. The extra only pulls in `annotated-types` (one of pydantic's dependencies, which #136 evicted), and nothing in smpclient validates a `u8` at runtime: `cast()` is a no-op, and the annotation is never read. Verified: `camas check` and `camas matrix` (3.10-3.14) green. A clean `uv pip install .` into a fresh 3.10 venv (what the `transport-extras` CI job does) has no `annotated_types`; `import smpclient` succeeds and `wrapping_sequence()` wraps 255 -> 0, with `u8` unbound at runtime. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
refactor: drop the runtime `u8 = int` shim; `u8` is typing-only
This branch has not been deployed
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.
Warning
LLM Disclosure
This PR was opened by
claude-opus-5-5[1m]on behalf of @JPHutchins, who asked for a draft release PR from thescreaming-goblinbreaking-release trunk intomain, mirroring JPHutchins/smp#66 and linked to the epic.Caution
This is a BREAKING release. It ports smpclient onto smp's breaking
Frame[T]/msgspecmessage API and reworks the client/transport lifecycle. smp releases first and smpmgr follows in lockstep. Merging this tomainis the breaking version bump.What this is
screaming-goblinis smpclient's breaking-release integration trunk for the transport-subcommands epic intercreate/smpmgr#103. Breaking work lands here, and non-breaking fixes land onmainand merge forward. This draft is the staging point for the release intomain. Keep it in draft until the breaking set is complete and smp has released.Upstream counterpart: JPHutchins/smp#66 (smp's
screaming-goblin).Included so far
request()on smp'sFrame[T]/msgspecmessages, andsmpclient/requests/**deleted in favor of smp's request types (closed Remove the generic request/response pattern #124).smpclient.genericsis folded intosmpclient, and the client owns its SMP sequence space (breaking: the SMP client owns the sequence space (closes #70) JPHutchins/smp#71).u8without a runtime shim (refactor: drop the runtimeu8 = intshim;u8is typing-only #138):types_bits.u8is typing-only, with notypes-bits[rt]dependency.In progress
async withbrackets,SMPClient.connect()goes away, and MCUmgr params negotiation becomes the transport's opt-in sizing policy. It also carries the per-transport options for Bleak transport options #90 and Add HCI option to BLE Transport #103.Landed on
mainand merged forwardmcuboot.py), fix(udp): cap the payload at the server's advertised buffer #142 (UDP respects the server's buffer).Before this leaves draft
screaming-goblin(breaking: screaming-goblin JPHutchins/smp#66)pyproject.toml: re-pinsmp @ git+…@screaming-goblinto that release (TODO(screaming-goblin))pyproject.toml: drop[tool.hatch.metadata] allow-direct-references = true, which exists only for that pinPart of the epic: intercreate/smpmgr#103
🤖 Generated with Claude Code