dev/2.1.0 - #50
Conversation
…ted servicesfeat: add join request confirm and decline APIs
feat: add new user attributes and add missing AuthFlow
📝 WalkthroughWalkthroughSingle checkpoint PR: updates CI/dev tooling, refactors 2FA enums/payloads and auth service/flows, adds Bots API and domain models (InitData, Member, Presence), implements chat join-request APIs, fixes TCP framing/sequence behavior, adds upload proxy/error handling, and includes a large testsuite and fixtures. ChangesConsolidated checkpoint
Estimated code review effort 🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
tests/api/test_chat_user_self_session_services.py (1)
23-31: ⚡ Quick winInclude
check_2fain the BaseMixin surface assertion.The PR adds
check_2fa, but this smoke test does not guard that exposure.✅ Suggested test update
for method_name in ( "get_join_requests", "confirm_join_requests", "confirm_join_request", "decline_join_requests", "decline_join_request", "get_bot_init_data", + "check_2fa", "change_password", ):🤖 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 `@tests/api/test_chat_user_self_session_services.py` around lines 23 - 31, The surface assertion for BaseMixin in tests/api/test_chat_user_self_session_services.py is missing the newly added method "check_2fa"; update the tuple iterated over by the test (the for loop that defines method_name) to include "check_2fa" so the smoke test asserts that BaseMixin exposes that method (i.e., add "check_2fa" to the list alongside "get_join_requests", "confirm_join_requests", etc.).src/pymax/api/chats/payloads.py (1)
108-116: ⚡ Quick winHarden join-request discriminator typing.
typeis currently free-form in both models; using a typed constant prevents accidental invalid payloads and removes TODO debt.♻️ Proposed refactor
class FetchJoinRequests(CamelModel): chat_id: int - type: str = "JOIN_REQUEST" # ENUM!!!!! + type: Literal["JOIN_REQUEST"] = "JOIN_REQUEST" count: int = 100 class JoinRequestActionPayload(CamelModel): chat_id: int user_ids: list[int] - type: str = "JOIN_REQUEST" # TODO: ENUMM!!! + type: Literal["JOIN_REQUEST"] = "JOIN_REQUEST" show_history: bool | None = True operation: ChatMemberOperation🤖 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/api/chats/payloads.py` around lines 108 - 116, The 'type' fields are free-form strings and should be hardened to a typed constant: define a small enum or typing.Literal (e.g., enum JoinRequestType(Enum) { JOIN_REQUEST = "JOIN_REQUEST" } or JoinRequestType = Literal["JOIN_REQUEST"]) and update the payload models to use that type instead of plain str (replace occurrences of type: str = "JOIN_REQUEST" in JoinRequestActionPayload and the other join-request payload model with type: JoinRequestType = JoinRequestType.JOIN_REQUEST or type: JoinRequestType = "JOIN_REQUEST" if using Literal), and add the necessary import for typing/Literal or Enum.tests/api/test_upload_service.py (1)
32-50: ⚡ Quick winAssert proxy propagation in upload tests.
These tests validate upload behavior, but they don’t verify that
UploadServicepassesapp.config.proxyintoaiohttp.ClientSession, which is a key part of this PR’s feature scope.✅ Suggested test enhancement
class FakeHttpSession: posts: list[dict] = [] + init_kwargs: list[dict] = [] response = FakeHttpResponse( 200, {"photos": {"photo-1": {"token": "uploaded"}}} ) def __init__(self, *args, **kwargs) -> None: self.args = args self.kwargs = kwargs + self.__class__.init_kwargs.append(kwargs) @@ monkeypatch.setattr( "pymax.api.uploads.service.aiohttp.ClientSession", FakeHttpSession, ) FakeHttpSession.posts = [] + FakeHttpSession.init_kwargs = [] @@ assert result.photo_token == "uploaded" + assert FakeHttpSession.init_kwargs[0]["proxy"] == app.config.proxy @@ monkeypatch.setattr( "pymax.api.uploads.service.aiohttp.ClientSession", FakeHttpSession, ) + FakeHttpSession.init_kwargs = [] @@ assert result.video_id == 10 + assert FakeHttpSession.init_kwargs[0]["proxy"] == app.config.proxy @@ monkeypatch.setattr( "pymax.api.uploads.service.aiohttp.ClientSession", FakeHttpSession, ) + FakeHttpSession.init_kwargs = [] @@ assert result.file_id == 11 + assert FakeHttpSession.init_kwargs[0]["proxy"] == app.config.proxyAlso applies to: 53-80, 104-146, 148-188
🤖 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 `@tests/api/test_upload_service.py` around lines 32 - 50, Update the tests to assert that the proxy value from app.config.proxy is forwarded into aiohttp.ClientSession by the UploadService: in the FakeHttpSession (used in tests at FakeHttpSession) capture the kwargs passed to the session constructor or to post and add an assertion that those kwargs include the expected proxy (app.config.proxy) value; locate where tests instantiate UploadService and patch/monkeypatch aiohttp.ClientSession to return FakeHttpSession, then assert the proxy was included in the captured constructor/post kwargs so UploadService actually propagates app.config.proxy into aiohttp.ClientSession.
🤖 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 @.github/workflows/publish.yml:
- Around line 16-23: The workflow uses third-party actions unpinned and leaves
credentials persisted; update both jobs (release-checks and release-build) to
pin actions/checkout@v4 and astral-sh/setup-uv@v4 to immutable references
(commit SHAs or fully-qualified tags) instead of floating tags, and add
persist-credentials: false to the actions/checkout invocation(s) to prevent
leaking workflow tokens; ensure you update every occurrence of the actions
(actions/checkout and astral-sh/setup-uv) in the workflow file so all uses are
pinned and credential persistence is disabled.
In `@src/pymax/connection/connection.py`:
- Around line 148-167: The receive loop currently sets self._connection_lost =
True on EOFError, TimeoutError, and generic Exception but leaves self._is_open
True so send() can be called; update the exception handling in
connection.receive loop to also mark the manager closed by setting self._is_open
= False (or call the existing close/mark-closed helper if one exists)
immediately after cancelling requests (e.g., in the EOFError, TimeoutError, and
generic Exception branches where self.requests.cancel_all(...) and
self._connection_lost = True are set) so no further sends are allowed against a
dead connection.
In `@src/pymax/types/domain/presence.py`:
- Around line 7-15: The model defines seen as a required int but the docstring
states it may be absent; update the annotation for seen to be optional (e.g.,
change seen: int to seen: Optional[int]) and give it a default of None so
parsing won't fail when the server omits it; add the necessary typing import
(from typing import Optional) and adjust any constructor/dataclass/pydantic
field declarations in presence.py (look for the seen and status declarations) to
reflect Optional[int] with default None.
In `@tests/api/test_auth_service.py`:
- Around line 238-240: Test currently overwrites the invalid app.me immediately,
so add an assertion that the invalid-shaped value is tested: call and assert
await app.api.auth.check_2fa() is False right after setting app.me = {"not": "a
profile"} (using the same app.me symbol), then reset app.me and
app.api.auth.app.me to None and assert again that await app.api.auth.check_2fa()
is False to cover both the malformed and None code paths.
In `@tests/connection/test_connection.py`:
- Line 137: The test passes a synchronous on_event lambda which returns None,
but ConnectionManager._dispatch_event awaits self.on_event(frame), causing await
None and hiding handler errors; change the test to provide an async callable
(e.g., async def on_event(event): events.append(event) or an async
lambda-equivalent) and pass that to the ConnectionManager instantiation
(reference on_event parameter in the test and ConnectionManager._dispatch_event)
so the awaited handler is awaitable and exceptions propagate into the manager's
exception handling path.
In `@tests/connection/test_readers_and_transports.py`:
- Line 101: The test currently asserts transport.connected is True after calling
await transport.close(); change this to assert transport.connected is False to
verify the transport's closed state. Locate the test in
tests/connection/test_readers_and_transports.py around the await
transport.close() call and update the assertion referencing the
transport.connected property so the close lifecycle is properly validated.
---
Nitpick comments:
In `@src/pymax/api/chats/payloads.py`:
- Around line 108-116: The 'type' fields are free-form strings and should be
hardened to a typed constant: define a small enum or typing.Literal (e.g., enum
JoinRequestType(Enum) { JOIN_REQUEST = "JOIN_REQUEST" } or JoinRequestType =
Literal["JOIN_REQUEST"]) and update the payload models to use that type instead
of plain str (replace occurrences of type: str = "JOIN_REQUEST" in
JoinRequestActionPayload and the other join-request payload model with type:
JoinRequestType = JoinRequestType.JOIN_REQUEST or type: JoinRequestType =
"JOIN_REQUEST" if using Literal), and add the necessary import for
typing/Literal or Enum.
In `@tests/api/test_chat_user_self_session_services.py`:
- Around line 23-31: The surface assertion for BaseMixin in
tests/api/test_chat_user_self_session_services.py is missing the newly added
method "check_2fa"; update the tuple iterated over by the test (the for loop
that defines method_name) to include "check_2fa" so the smoke test asserts that
BaseMixin exposes that method (i.e., add "check_2fa" to the list alongside
"get_join_requests", "confirm_join_requests", etc.).
In `@tests/api/test_upload_service.py`:
- Around line 32-50: Update the tests to assert that the proxy value from
app.config.proxy is forwarded into aiohttp.ClientSession by the UploadService:
in the FakeHttpSession (used in tests at FakeHttpSession) capture the kwargs
passed to the session constructor or to post and add an assertion that those
kwargs include the expected proxy (app.config.proxy) value; locate where tests
instantiate UploadService and patch/monkeypatch aiohttp.ClientSession to return
FakeHttpSession, then assert the proxy was included in the captured
constructor/post kwargs so UploadService actually propagates app.config.proxy
into aiohttp.ClientSession.
🪄 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: cba9837a-3b1c-4464-b988-d110ff537cc3
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (78)
.github/workflows/publish.yml.pre-commit-config.yamlREADME.mddocs/index.rstdocs/release-2-1-0.rstpyproject.tomlsrc/pymax/api/auth/enums.pysrc/pymax/api/auth/payloads.pysrc/pymax/api/auth/service.pysrc/pymax/api/bots/__init__.pysrc/pymax/api/bots/payloads.pysrc/pymax/api/bots/service.pysrc/pymax/api/chats/enums.pysrc/pymax/api/chats/payloads.pysrc/pymax/api/chats/service.pysrc/pymax/api/facade.pysrc/pymax/api/messages/payloads.pysrc/pymax/api/messages/service.pysrc/pymax/api/self/service.pysrc/pymax/api/session/payloads.pysrc/pymax/api/uploads/models.pysrc/pymax/api/uploads/payloads.pysrc/pymax/api/uploads/service.pysrc/pymax/api/users/service.pysrc/pymax/app.pysrc/pymax/auth/qr.pysrc/pymax/auth/sms.pysrc/pymax/base.pysrc/pymax/client.pysrc/pymax/client_web.pysrc/pymax/config.pysrc/pymax/connection/connection.pysrc/pymax/connection/readers/tcp.pysrc/pymax/dispatch/dispatcher.pysrc/pymax/dispatch/mapping.pysrc/pymax/dispatch/router.pysrc/pymax/files/base.pysrc/pymax/formatting/markdown.pysrc/pymax/infra/auth.pysrc/pymax/infra/base.pysrc/pymax/infra/bots.pysrc/pymax/infra/chat.pysrc/pymax/protocol/tcp/compression.pysrc/pymax/protocol/tcp/framing.pysrc/pymax/protocol/tcp/payload.pysrc/pymax/protocol/tcp/protocol.pysrc/pymax/protocol/ws/protocol.pysrc/pymax/session/protocol.pysrc/pymax/session/store.pysrc/pymax/telemetry/navigation.pysrc/pymax/telemetry/service.pysrc/pymax/transport/tcp.pysrc/pymax/transport/websocket.pysrc/pymax/types/domain/__init__.pysrc/pymax/types/domain/bots.pysrc/pymax/types/domain/error.pysrc/pymax/types/domain/folder.pysrc/pymax/types/domain/login.pysrc/pymax/types/domain/member.pysrc/pymax/types/domain/presence.pysrc/pymax/types/domain/sync.pysrc/pymax/types/domain/user.pytests/__init__.pytests/api/test_auth_service.pytests/api/test_chat_user_self_session_services.pytests/api/test_message_service.pytests/api/test_upload_service.pytests/app/test_app_runtime.pytests/auth/test_auth_flows.pytests/conftest.pytests/connection/test_connection.pytests/connection/test_readers_and_transports.pytests/dispatch/test_dispatcher.pytests/domain/test_bound_models.pytests/files/test_files_and_formatting.pytests/protocol/test_protocols.pytests/session/test_store.pytests/telemetry/test_telemetry.py
💤 Files with no reviewable changes (1)
- src/pymax/transport/websocket.py
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up uv | ||
| uses: astral-sh/setup-uv@v4 | ||
| with: | ||
| python-version: "3.10" | ||
| enable-cache: true | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant workflow
echo "== publish.yml =="
sed -n '1,120p' .github/workflows/publish.yml
# Check if other workflows reference these actions unpinned
echo
echo "== Other unpinned actions/checkout@v4 or setup-uv@v4 usages =="
rg -n --hidden --glob '!.git/*' "uses:\s*actions/checkout@v4|uses:\s*astral-sh/setup-uv@v4" .github/workflows || true
# Check whether persist-credentials is set anywhere in the workflow
echo
echo "== persist-credentials occurrences in publish.yml =="
rg -n "persist-credentials" .github/workflows/publish.yml || trueRepository: MaxApiTeam/PyMax
Length of output: 2134
🌐 Web query:
actions/checkout@v4 persist-credentials default value
💡 Result:
The default value for the persist-credentials input in actions/checkout@v4 is true [1][2]. When this option is set to true (the default), the action configures the Git repository with the provided authentication token (such as GITHUB_TOKEN) or SSH key in the local.git/config file [1][3]. This allows subsequent steps in the workflow to perform authenticated Git commands [1]. The token is removed during the post-job cleanup process [1]. To disable this behavior and prevent the credentials from being persisted in the local Git configuration, you must explicitly set the input to false: - uses: actions/checkout@v4 with: persist-credentials: false
Citations:
- 1: https://github.com/actions/checkout/tree/v4
- 2: https://github.com/actions/checkout
- 3: https://yossarian.net/til/post/actions-checkout-can-leak-github-credentials/
Harden third-party action usage in publish workflow.
actions/checkout@v4andastral-sh/setup-uv@v4are used unpinned (appears in bothrelease-checksandrelease-buildjobs).actions/checkout@v4credential persistence is not disabled (nopersist-credentials: falsein workflow; default istrue), weakening release supply-chain hardening.
🔒 Suggested hardening diff
- - uses: actions/checkout@v4
+ - uses: actions/checkout@<FULL_LENGTH_COMMIT_SHA>
+ with:
+ persist-credentials: false
- name: Set up uv
- uses: astral-sh/setup-uv@v4
+ uses: astral-sh/setup-uv@<FULL_LENGTH_COMMIT_SHA>🧰 Tools
🪛 zizmor (1.25.2)
[warning] 16-16: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 16-16: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 19-19: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 19-19: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): this step
(cache-poisoning)
🤖 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 @.github/workflows/publish.yml around lines 16 - 23, The workflow uses
third-party actions unpinned and leaves credentials persisted; update both jobs
(release-checks and release-build) to pin actions/checkout@v4 and
astral-sh/setup-uv@v4 to immutable references (commit SHAs or fully-qualified
tags) instead of floating tags, and add persist-credentials: false to the
actions/checkout invocation(s) to prevent leaking workflow tokens; ensure you
update every occurrence of the actions (actions/checkout and astral-sh/setup-uv)
in the workflow file so all uses are pinned and credential persistence is
disabled.
| app.me = {"not": "a profile"} | ||
| app.me = app.api.auth.app.me = None | ||
| assert await app.api.auth.check_2fa() is False |
There was a problem hiding this comment.
Assert the invalid app.me shape before resetting it.
Line 238 sets a non-profile value, but Line 239 immediately resets it to None, so Line 240 only re-tests the None path.
✅ Suggested fix
app.me = {"not": "a profile"}
- app.me = app.api.auth.app.me = None
- assert await app.api.auth.check_2fa() is False
+ assert await app.api.auth.check_2fa() is False
+ app.me = app.api.auth.app.me = None
+ assert await app.api.auth.check_2fa() is False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| app.me = {"not": "a profile"} | |
| app.me = app.api.auth.app.me = None | |
| assert await app.api.auth.check_2fa() is False | |
| app.me = {"not": "a profile"} | |
| assert await app.api.auth.check_2fa() is False | |
| app.me = app.api.auth.app.me = None | |
| assert await app.api.auth.check_2fa() is False |
🤖 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 `@tests/api/test_auth_service.py` around lines 238 - 240, Test currently
overwrites the invalid app.me immediately, so add an assertion that the
invalid-shaped value is tested: call and assert await app.api.auth.check_2fa()
is False right after setting app.me = {"not": "a profile"} (using the same
app.me symbol), then reset app.me and app.api.auth.app.me to None and assert
again that await app.api.auth.check_2fa() is False to cover both the malformed
and None code paths.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/publish.yml (1)
16-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin GitHub Actions to immutable SHAs in release workflow.
Good fix on
persist-credentials: false, but release jobs still use floating refs (@v4,@release/v1). Please pin all third-party actions in this workflow to full commit SHAs to harden the publish pipeline.🔒 Minimal hardening pattern
- - uses: actions/checkout@v4 + - uses: actions/checkout@<FULL_LENGTH_SHA> with: persist-credentials: false - - name: Set up uv - uses: astral-sh/setup-uv@v4 + - name: Set up uv + uses: astral-sh/setup-uv@<FULL_LENGTH_SHA>#!/bin/bash set -euo pipefail # Verify all action refs in publish workflow and highlight non-SHA pins. awk '/uses: /{print NR ":" $0}' .github/workflows/publish.yml echo echo "Potentially unpinned refs (not @<40-hex-sha>):" rg -n 'uses:\s*[^@]+@(?!(?:[a-f0-9]{40})\b)' .github/workflows/publish.ymlAlso applies to: 43-48
🤖 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 @.github/workflows/publish.yml around lines 16 - 21, Replace floating action refs with immutable commit SHAs: update every uses: entry such as actions/checkout@v4 and astral-sh/setup-uv@v4 (and any uses: referencing `@release/v1`) to the corresponding full 40-hex commit SHA for the exact versions you want to pin; ensure all non-SHA pins in .github/workflows/publish.yml are converted so each uses: reference ends with @<40-hex-sha>, and verify there are no remaining floating refs in the release job.
🤖 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.
Duplicate comments:
In @.github/workflows/publish.yml:
- Around line 16-21: Replace floating action refs with immutable commit SHAs:
update every uses: entry such as actions/checkout@v4 and astral-sh/setup-uv@v4
(and any uses: referencing `@release/v1`) to the corresponding full 40-hex commit
SHA for the exact versions you want to pin; ensure all non-SHA pins in
.github/workflows/publish.yml are converted so each uses: reference ends with
@<40-hex-sha>, and verify there are no remaining floating refs in the release
job.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 856cdddc-c765-4c6a-a58c-1b86c2b395fe
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.github/workflows/publish.ymlpyproject.tomlsrc/pymax/__init__.pysrc/pymax/connection/connection.pysrc/pymax/transport/tcp.pysrc/pymax/types/domain/presence.pytests/api/test_auth_service.pytests/connection/test_connection.pytests/connection/test_readers_and_transports.py
✅ Files skipped from review due to trivial changes (1)
- src/pymax/init.py
| - name: Check lint | ||
| run: uv run ruff check src tests | ||
|
|
||
| - name: Check formatting | ||
| run: uv run ruff format --check src tests | ||
|
|
||
| - name: Run tests | ||
| run: uv run pytest |
There was a problem hiding this comment.
Лучше запускать formatting, lint и тесты на любой PR, чтобы не пропускать проблемы до мержа. В отдельный action их перенести.
Также стоит явно указать src и tests в конфигурации ruff.
Описание
Кратко, что делает этот PR. Например, добавляет новый метод, исправляет баг, улучшает документацию.
PyMax 2.1.0
Релиз про новые API-методы, стабильность TCP-клиента и нормальную release-гигиену.
Added
get_join_requests()confirm_join_request()confirm_join_requests()decline_join_request()decline_join_requests()check_2fa()change_password()authorize_qr_login(qr_link)get_bot_init_data(bot_id, chat_id, start_param=None)Member,Presence,InitData.Fixed
ver:1,cmd:1,seq:2,opcode:2,len/cof:4.seqбольше не падает после255; диапазон теперь0..65535.ExtraConfig.proxyдля HTTP upload-запросов.MaxApiError.titleиMaxApiError.localized_messageтеперь могут отсутствовать в payload.Changed
Capabilityразделен наProfileOptionsиTwoFactorAction.BotsService.Тип изменений
Связанные задачи / Issue
Ссылка на issue, если есть: #
#49
Тестирование
Покажите пример кода, который проверяет изменения:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores