Skip to content

fix(client): readable SMPValidationException on unparseable frame#116

Merged
JPHutchins merged 2 commits into
mainfrom
fix/validation-error-diagnostics
Jun 12, 2026
Merged

fix(client): readable SMPValidationException on unparseable frame#116
JPHutchins merged 2 commits into
mainfrom
fix/validation-error-diagnostics

Conversation

@JPHutchins

Copy link
Copy Markdown
Collaborator

Summary

Improves SMP frame-parse failure diagnostics: when a response frame can't be parsed as the request's Response, ErrorV1, or ErrorV2, the client now raises a dedicated SMPValidationException carrying a readable summary + a hexdump and per-type Pydantic error breakdown — instead of the current code path, which is itself broken on main:

raise ValidationError(error_message)  # TypeError: __new__() missing 1 required positional argument: 'line_errors'

This revives @JaagupAverin's work from #88 / #91 (both unmerged; the codebase has since moved smpclient/src/smpclient/ and envr/black → uv/ruff), rebased onto current main with his authorship preserved.

Commits

  1. chore: Cleanup validation error message. — authored by @JaagupAverin (cherry-picked, adapted to the src/ layout). The substantive fix + diagnostics.
  2. refactor(client): functional cleanup of validation error diagnostics — applies project standards and the Jp/fix linter (ignore) #91 review:
    • Functional/immutable formatters (pure inner row() helpers over generators, no accumulator mutation).
    • Parse failures collected in an immutable tuple[type[Response], ValidationError] rather than dict[Type, ...] → resolves the Copilot mypy-invariance flag.
    • Fixes the stray trailing ) on each error line; corrects the misleading _hexdump docstring (no stdlib hexdump builtin).
    • pydantic_core.ErrorDetails typing; raise ... from None (per @JaagupAverin's suggestion).
    • Adds a regression test asserting the new exception type and its header/frame/per-type diagnostics.

Example output

Frame could not be parsed as any of:
	['ResetWriteResponse', 'OSManagementErrorV1', 'OSManagementErrorV2']
Header:
	Header(op=<OP.WRITE_RSP: 3>, version=<Version.V2: 1>, ... group_id=1, sequence=0, command_id=1)
Frame:
	0000  0b 00 00 06 00 01 00 01 a1 63 6f 66 66 00        .........coff.
Errors:
	Could not be parsed as ResetWriteResponse because 1 error(s):
		[extra_forbidden] Extra inputs are not permitted: off; input: 0
	...

Testing

  • task lint (ruff + pydoclint) ✓
  • task typecheck (mypy) ✓
  • task test → 339 passed, 14 skipped (+1 new test)

Supersedes #91 and revives #88.

🤖 Generated with Claude Code

JaagupAverin and others added 2 commits June 12, 2026 13:21
Additionally fixes an issue where `ValidationError(error)` constructor would re-raise an exception because it requires additional arguments.

(cherry picked from commit 1fac50d)
Builds on Jaagup Averin's validation error message cleanup, applying the
project's standards and addressing the PR #91 review:

- Rewrite the hexdump and error formatters in a functional, immutable
  style (no accumulator mutation); pure inner `row()` helpers joined over
  generators.
- Accumulate the per-type parse failures in an immutable
  `tuple[type[Response], ValidationError]` instead of a `dict[Type, ...]`,
  resolving the `mypy` invariance concern.
- Fix the stray trailing `)` in each formatted validation error line.
- Replace the misleading `_hexdump` docstring (there is no stdlib hexdump
  builtin) with an accurate description.
- Type the error details via `pydantic_core.ErrorDetails`.
- Return `(summary, details)` and raise `SMPValidationException` directly
  so `pydoclint` can match the documented exception.
- Add a regression test asserting the new exception type and that its
  `details` carry the header/frame/per-type diagnostics.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 12, 2026 20:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves SMP frame-parse failure diagnostics by introducing a dedicated SMPValidationException and emitting a readable summary plus detailed per-type Pydantic validation breakdown (including a hexdump) when a response frame can’t be parsed as the expected Response, ErrorV1, or ErrorV2.

Changes:

  • Add SMPValidationException and raise it (instead of the currently broken ValidationError(...) path) when none of the candidate response types validate.
  • Add formatting helpers to produce a hexdump and structured per-type Pydantic error detail output.
  • Add a regression test asserting the new exception type and the presence of key diagnostics sections.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/smpclient/__init__.py Adds hexdump + validation error formatting helpers and raises/logs SMPValidationException when parsing fails for all response/error types.
src/smpclient/exceptions.py Introduces the new SMPValidationException type carrying msg and details.
tests/test_smp_client.py Adds a regression test ensuring an unparseable frame raises SMPValidationException and includes expected diagnostics content.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/smpclient/__init__.py
Comment on lines +96 to +99
summary: Final = (
"\nFrame could not be parsed as any of:\n"
f"\t{[response.__name__ for response, _ in errors]}\n"
)
Comment thread src/smpclient/__init__.py
Comment on lines +257 to +259
summary, details = _validation_failure(header, frame, tuple(errors))
logger.error(summary + details)
raise SMPValidationException(summary, details) from None
@JPHutchins JPHutchins merged commit f0d612a into main Jun 12, 2026
30 checks passed
@JPHutchins JPHutchins deleted the fix/validation-error-diagnostics branch June 12, 2026 20:32
@JPHutchins JPHutchins mentioned this pull request Jun 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants