Dev/2.1.3 - #60
Merged
Merged
Conversation
Animoji/sticker elements arrive from the server without a `url` field, so the strict `url: str` validation raises ValidationError and breaks login sync. The server returns such elements without `url`, so the model is stricter than the protocol itself.
Fix photo URL validation with query parameters
fix(types): make ElementAttributes.url optional
Contributor
📝 WalkthroughWalkthroughPyMax 2.1.3 bumps the version and introduces unknown attachment type handling, connection lifecycle callbacks for graceful shutdown, optional fields for attachments and elements, URL query-string filtering in photo validation, and configurable logging with a force-override mode. ChangesPyMax 2.1.3 Release
Sequence DiagramsequenceDiagram
participant App
participant ConnectionManager
participant ReceiveLoop
participant Handler
App->>ConnectionManager: init(on_close=on_connection_lost)
ConnectionManager->>ConnectionManager: store on_close callback, _close_reported=False
Note over ReceiveLoop: Normal operation
Note over ReceiveLoop: Connection error occurs
ReceiveLoop->>ConnectionManager: error in _recv_loop
ConnectionManager->>ConnectionManager: cancel pending requests<br/>set _connection_lost=True
ConnectionManager->>ConnectionManager: _mark_closed(exc)
activate ConnectionManager
ConnectionManager->>ConnectionManager: set _is_open=False
ConnectionManager->>Handler: on_close(ConnectionError)
deactivate ConnectionManager
Handler->>App: on_connection_lost triggered
activate App
App->>App: set started=False
App->>App: cancel ping task
deactivate App
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/pymax/protocol/tcp/payload.py (2)
28-28: 💤 Low valueOptional: Remove unnecessary defensive
or b"".
msgpack.packb()always returnsbytes, neverNoneor an empty falsy value (e.g.,msgpack.packb({})returnsb'\x80'). Theor b""fallback is unnecessary.♻️ Simplify to direct return
- return msgpack.packb(self._to_msgpack_value(payload), use_bin_type=True) or b"" + return msgpack.packb(self._to_msgpack_value(payload), use_bin_type=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pymax/protocol/tcp/payload.py` at line 28, Remove the unnecessary defensive fallback in the payload serialization: change the return that currently does "msgpack.packb(self._to_msgpack_value(payload), use_bin_type=True) or b''" to simply return the result of msgpack.packb directly (i.e., "return msgpack.packb(self._to_msgpack_value(payload), use_bin_type=True)"); this touches the serialization code that calls msgpack.packb and uses _to_msgpack_value(payload).
48-56: Rework ExtraData concern: current behavior is intentional and test-covered
src/pymax/protocol/tcp/payload.pycatchesmsgpack.exceptions.ExtraData, logs debug details, and returnse.unpacked(trailing msgpack objects in the payload are ignored).tests/protocol/test_protocols.py::test_msgpack_codec_uses_first_dict_when_stream_has_extra_dataasserts this exact behavior by decoding concatenated msgpack objects to only the first ({"ok": True}).- Any “data loss” here only applies if the protocol allows multiple msgpack objects within a single TCP payload frame; if that’s malformed/unexpected, consider warning or rejecting on
ExtraDatato avoid silent truncation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pymax/protocol/tcp/payload.py` around lines 48 - 56, The except block catching msgpack.exceptions.ExtraData (where e is used and e.unpacked is returned, with payload_bytes logged) is intentional and covered by tests; preserve the current behavior (return e.unpacked) but add a clear inline comment explaining that trailing msgpack objects are intentionally ignored (reference the test that asserts this behavior), and optionally add a TODO or feature flag comment suggesting rejecting/warning on ExtraData if a stricter protocol is desired; do not change the return semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pymax/protocol/tcp/payload.py`:
- Line 37: MsgpackPayloadCodec.decode currently returns Any which hides the
implicit dict contract expected by tests and by TcpPayloadDecoder.decode; change
MsgpackPayloadCodec.decode to return dict[str, Any] and implement runtime
handling so when the top-level msgpack value is a map you return a dict with
string keys (decode/convert bytes keys to str), and if the top-level value is
not a map either raise a clear exception or wrap it into a dict with a known key
(so callers like TcpPayloadDecoder.decode and tests/protocol/test_protocols.py
always get dict[str, Any]); ensure key-normalization logic matches
TcpPayloadDecoder.decode expectations.
---
Nitpick comments:
In `@src/pymax/protocol/tcp/payload.py`:
- Line 28: Remove the unnecessary defensive fallback in the payload
serialization: change the return that currently does
"msgpack.packb(self._to_msgpack_value(payload), use_bin_type=True) or b''" to
simply return the result of msgpack.packb directly (i.e., "return
msgpack.packb(self._to_msgpack_value(payload), use_bin_type=True)"); this
touches the serialization code that calls msgpack.packb and uses
_to_msgpack_value(payload).
- Around line 48-56: The except block catching msgpack.exceptions.ExtraData
(where e is used and e.unpacked is returned, with payload_bytes logged) is
intentional and covered by tests; preserve the current behavior (return
e.unpacked) but add a clear inline comment explaining that trailing msgpack
objects are intentionally ignored (reference the test that asserts this
behavior), and optionally add a TODO or feature flag comment suggesting
rejecting/warning on ExtraData if a stricter protocol is desired; do not change
the return semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bcc99940-a2d8-4b5f-977b-7cb52569b7cc
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
docs/conf.pydocs/index.rstdocs/release-2-1-3.rstpyproject.tomlsrc/pymax/__init__.pysrc/pymax/app.pysrc/pymax/client.pysrc/pymax/connection/connection.pysrc/pymax/files/photo.pysrc/pymax/logging.pysrc/pymax/protocol/tcp/payload.pysrc/pymax/protocol/tcp/protocol.pysrc/pymax/types/domain/attachments/__init__.pysrc/pymax/types/domain/attachments/audio.pysrc/pymax/types/domain/attachments/enums.pysrc/pymax/types/domain/attachments/unknown.pysrc/pymax/types/domain/attachments/video.pysrc/pymax/types/domain/element.pysrc/pymax/types/domain/message.pytests/app/test_app_runtime.pytests/connection/test_connection.pytests/domain/test_message_models.pytests/files/test_files_and_formatting.pytests/test_logging.py
| return list(unpacker) | ||
|
|
||
| def decode(self, payload_bytes: bytes) -> dict[Any, Any]: | ||
| def decode(self, payload_bytes: bytes) -> Any: |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find usages of MsgpackPayloadCodec.decode to verify compatibility.
# Find instantiations and calls
rg -nP 'MsgpackPayloadCodec\(\)\.decode|serializer\.decode' --type=py -C3Repository: MaxApiTeam/PyMax
Length of output: 460
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate the codec class definition
rg -n --hidden --no-ignore -S "class MsgpackPayloadCodec" src
# 2) Find instantiations/usages of MsgpackPayloadCodec
rg -n --hidden --no-ignore -S "MsgpackPayloadCodec\s*\(" src -g'*.py'
# 3) Find call sites of `.decode(` that involve MsgpackPayloadCodec in the same vicinity
rg -n --hidden --no-ignore -S "MsgpackPayloadCodec.*\.decode\(" src -g'*.py' -C3 || true
# 4) Fallback: find all `.decode(` call sites in the codec module(s) where MsgpackPayloadCodec is used
# (kept narrow to avoid noise)
rg -n --hidden --no-ignore -S "\.decode\(" src/pymax/protocol/tcp -g'*.py' -C2Repository: MaxApiTeam/PyMax
Length of output: 1276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show MsgpackPayloadCodec definition + decode signature/implementation
echo "=== payload.py around MsgpackPayloadCodec ==="
nl -ba src/pymax/protocol/tcp/payload.py | sed -n '1,220p' | sed -n '1,220p'
echo "=== protocol.py around MsgpackPayloadCodec instantiation/usage ==="
nl -ba src/pymax/protocol/tcp/protocol.py | sed -n '1,140p'Repository: MaxApiTeam/PyMax
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== payload.py (top 200 lines) ==="
sed -n '1,200p' src/pymax/protocol/tcp/payload.py
echo "=== payload.py (around lines 60-140) ==="
sed -n '60,160p' src/pymax/protocol/tcp/payload.py
echo "=== protocol.py (top 140 lines) ==="
sed -n '1,140p' src/pymax/protocol/tcp/protocol.pyRepository: MaxApiTeam/PyMax
Length of output: 7618
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for all usages/instantiations of MsgpackPayloadCodec
rg -n --hidden --no-ignore -S "MsgpackPayloadCodec" . -g'*.py'
# Look for direct call patterns (new/alt spacing)
rg -n --hidden --no-ignore -S "MsgpackPayloadCodec\s*\(\s*\)\s*\.decode\s*\(" . -g'*.py' || true
rg -n --hidden --no-ignore -S "\.decode\s*\(\s*.*payload_bytes" src/pymax/protocol -g'*.py' || true
# Heuristic: variables named serializer/codec typed/assigned from MsgpackPayloadCodec, then decode called
rg -n --hidden --no-ignore -S "(serializer|codec)\s*=\s*MsgpackPayloadCodec\s*\(\s*\)|MsgpackPayloadCodec\s*\(\s*\)\s*$" . -g'*.py' -C2
# Check if MsgpackPayloadCodec is exported from a public module
rg -n --hidden --no-ignore -S "from .*payload import .*MsgpackPayloadCodec|MsgpackPayloadCodec" src/pymax/protocol -g'*.py' -C2Repository: MaxApiTeam/PyMax
Length of output: 3284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' tests/protocol/test_protocols.pyRepository: MaxApiTeam/PyMax
Length of output: 4197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find likely local variables assigned from MsgpackPayloadCodec() and where they call .decode(...)
rg -n --hidden --no-ignore -S "(\w+)\s*=\s*MsgpackPayloadCodec\(\)" . -g'*.py'
# Find any "<var>.decode(" where <var> was assigned from MsgpackPayloadCodec()
rg -n --hidden --no-ignore -S "\b(\w+)\.decode\(" tests src -g'*.py'Repository: MaxApiTeam/PyMax
Length of output: 1170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant part of connection.py where protocol.decode payload is used
sed -n '130,220p' src/pymax/connection/connection.py
# Also search for "payload" usage patterns and inspect nearby lines
rg -n --hidden --no-ignore -S "payload" src/pymax/connection -g'*.py' -C2Repository: MaxApiTeam/PyMax
Length of output: 4728
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --no-ignore -S "model_validate\(" src -g'*.py' || true
rg -n --hidden --no-ignore -S "frame\.payload|\.payload\b" src/pymax/connection -g'*.py' -C2 || true
rg -n --hidden --no-ignore -S "payload.*model" src/pymax/connection -g'*.py' -C2 || true
# Show connection handling around where inbound frames are dispatched/resolved
rg -n --hidden --no-ignore -S "def _handle_inbound|_dispatch_event|resolve\(" src/pymax/connection/connection.py -C2
sed -n '1,260p' src/pymax/connection/connection.pyRepository: MaxApiTeam/PyMax
Length of output: 10799
MsgpackPayloadCodec.decode return-type widening to Any is compatible with in-repo callers, but the dict contract is implicit.
- Direct in-repo callers:
tests/protocol/test_protocols.pyinstantiatesMsgpackPayloadCodecand assertscodec.decode(...)returns adictfor msgpack payloads encoded as maps. - Protocol decoding path:
TcpProtocolconsumes payloads only viaTcpPayloadDecoder.decode, which normalizes keys and returnsdict[str, Any]when the top-level msgpack value is a map. - Caveat:
MsgpackPayloadCodec.decodecan return non-dict top-level msgpack values, andTcpPayloadDecoder.decodeis annotated asdict[str, Any], so the dict shape is an unstated runtime contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pymax/protocol/tcp/payload.py` at line 37, MsgpackPayloadCodec.decode
currently returns Any which hides the implicit dict contract expected by tests
and by TcpPayloadDecoder.decode; change MsgpackPayloadCodec.decode to return
dict[str, Any] and implement runtime handling so when the top-level msgpack
value is a map you return a dict with string keys (decode/convert bytes keys to
str), and if the top-level value is not a map either raise a clear exception or
wrap it into a dict with a known key (so callers like TcpPayloadDecoder.decode
and tests/protocol/test_protocols.py always get dict[str, Any]); ensure
key-normalization logic matches TcpPayloadDecoder.decode expectations.
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
3 participants
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.
Описание
Готовит релиз PyMax 2.1.3 и фиксит стабильность парсинга живых payload-ов, lifecycle соединения и интеграцию logging.
Основное:
UnknownAttachmentдля неизвестных типов вложений;Photo(url=...)с query string;App.startedтеперь сбрасывается при потере соединения, ping-task отменяется, pending requests очищаются без unhandled future warnings;configure_logging()больше не перетирает logging host-приложения, но сохраняет pretty-логи из коробки;2.1.3.Тип изменений
Связанные задачи / Issue
Ссылка на issue, если есть: #
#58
#57
#59
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Behavior Changes
configure_logging()now respects existing logging handlers unlessforce=True.