Dev/2.2.0#64
Merged
Merged
Conversation
feat: update ci
fix: handle non-BMP UTF-16 characters in markdown formatting
Contributor
📝 WalkthroughWalkthroughPyMax 2.2.0 adds four new event types (message read, typing, presence, reaction), message retrieval and editing APIs, SMS registration completion via ChangesPyMax 2.2.0 Release
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pymax/session/store.py (2)
74-84:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix SQL injection risk in ALTER TABLE statement (line 84).
The
_ensure_columnmethod builds an SQL query using an f-string without parameterization. While the current hardcoded calls are safe, this pattern violates SQL injection prevention best practices and is vulnerable if the method is called with untrusted input.Although column names cannot be parameterized in SQL, they should be validated or safely quoted:
🔒️ Proposed fix using identifier quoting
async def _ensure_column( self, conn: aiosqlite.Connection, name: str, definition: str, ) -> None: async with conn.execute("PRAGMA table_info(sessions)") as cursor: columns = {row["name"] for row in await cursor.fetchall()} if name not in columns: - await conn.execute(f"ALTER TABLE sessions ADD COLUMN {name} {definition}") + # Use square brackets to safely quote identifier; definition is internal (hardcoded in _initialize_db) + await conn.execute(f"ALTER TABLE sessions ADD COLUMN [{name}] {definition}")Alternatively, if stricter validation is preferred:
async def _ensure_column( self, conn: aiosqlite.Connection, name: str, definition: str, ) -> None: + # Whitelist allowed column names to prevent injection + if name not in { + "mt_instance_id", "chats_sync", "contacts_sync", + "drafts_sync", "presence_sync", "config_hash" + }: + raise ValueError(f"Unknown session column: {name}") + async with conn.execute("PRAGMA table_info(sessions)") as cursor: columns = {row["name"] for row in await cursor.fetchall()} if name not in columns: - await conn.execute(f"ALTER TABLE sessions ADD COLUMN {name} {definition}") + await conn.execute(f"ALTER TABLE sessions ADD COLUMN {name} {definition}")🤖 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/session/store.py` around lines 74 - 84, The ALTER TABLE construction in _ensure_column builds SQL with an f-string which is unsafe; validate and safely quote the column identifier before interpolating and also validate the definition string. Update _ensure_column to: (1) validate name against a strict identifier regex (e.g. ^[A-Za-z_][A-Za-z0-9_]*$) and raise on mismatch; (2) escape and quote the identifier for SQLite by wrapping in double quotes and doubling any internal double quotes (name_escaped = '"' + name.replace('"', '""') + '"'); (3) validate definition against an allowed pattern or whitelist (e.g. only types/keywords you expect) or raise if it contains suspicious chars; then use conn.execute(f'ALTER TABLE sessions ADD COLUMN {name_escaped} {definition}') knowing the identifier is safe and definition has been validated. Ensure these checks are applied in the _ensure_column function and raise a clear error for invalid inputs.Source: Linters/SAST tools
147-164:⚠️ Potential issue | 🟡 MinorUpdate call-site check for
load_session_by_device_id.
load_session_by_device_iduses a parameterizedWHERE device_id = ?query as expected, but this repository never calls it outside unit tests (Apponly callsstore.load_session()on startup). If the protocol flow is supposed to retrieve sessions bydevice_idduring runtime/reconnect, this method isn’t currently wired—either connect the expected call path or remove/de-scope the unused API.🤖 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/session/store.py` around lines 147 - 164, The repository defines load_session_by_device_id in SessionStore but no runtime code calls it (only tests), so either wire it into the session/reconnect flow or remove it; to fix, locate the reconnect/startup path in App (where store.load_session() is called) and replace or augment that call to use store.load_session_by_device_id(device_id) when a device_id is available (ensure device_id is passed into the App startup/reconnect routine and handle None result the same way load_session() did), or if the repo design never requires device lookup by id, remove the load_session_by_device_id method and update its unit tests to use the public store.load_session() API instead.
🧹 Nitpick comments (1)
src/pymax/types/events/message.py (1)
67-74: Opcode-128 delete normalization expects top-levelchatIdin exercised web “removed message” payloads. The dispatcher test fixture for the web removed-message delete event supplies top-level"chatId"even when"message"omits it, so the currentdata.get("chatId", ...)path won’t fail in the covered case. For robustness against payloads wherechatIdexists only undermessage, add a fallback to extractchatId/chat_idfrommessagein the opcode-128 branch.🤖 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/types/events/message.py` around lines 67 - 74, In the opcode-128 branch where you extract message/message_id/chat_id, add a fallback to pull chatId/chat_id from the nested message when top-level chatId is missing: after computing message (and message_id), set chat_id = data.get("chatId", data.get("chat_id")) or (message.get("chatId", message.get("chat_id")) if isinstance(message, dict) else getattr(message, "chatId", getattr(message, "chat_id", None))); keep the existing early-return check that returns data if chat_id or message_id is still None. This ensures the branch handles payloads that only include chatId inside message.
🤖 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/tests.yml:
- Around line 22-23: Update the GitHub Actions workflow step that currently
reads "name: Checkout repository" with "uses: actions/checkout@v6" and any other
actions referenced by tag (e.g., "astral-sh/setup-uv@v8.1.0") to use specific
commit SHAs instead of tags, and add "with: persist-credentials: false" under
each actions/checkout step to disable credential persistence; ensure you replace
the tag references with the corresponding full commit SHA for actions/checkout
and astral-sh/setup-uv and add the persist-credentials setting for every
checkout step in the workflow.
---
Outside diff comments:
In `@src/pymax/session/store.py`:
- Around line 74-84: The ALTER TABLE construction in _ensure_column builds SQL
with an f-string which is unsafe; validate and safely quote the column
identifier before interpolating and also validate the definition string. Update
_ensure_column to: (1) validate name against a strict identifier regex (e.g.
^[A-Za-z_][A-Za-z0-9_]*$) and raise on mismatch; (2) escape and quote the
identifier for SQLite by wrapping in double quotes and doubling any internal
double quotes (name_escaped = '"' + name.replace('"', '""') + '"'); (3) validate
definition against an allowed pattern or whitelist (e.g. only types/keywords you
expect) or raise if it contains suspicious chars; then use conn.execute(f'ALTER
TABLE sessions ADD COLUMN {name_escaped} {definition}') knowing the identifier
is safe and definition has been validated. Ensure these checks are applied in
the _ensure_column function and raise a clear error for invalid inputs.
- Around line 147-164: The repository defines load_session_by_device_id in
SessionStore but no runtime code calls it (only tests), so either wire it into
the session/reconnect flow or remove it; to fix, locate the reconnect/startup
path in App (where store.load_session() is called) and replace or augment that
call to use store.load_session_by_device_id(device_id) when a device_id is
available (ensure device_id is passed into the App startup/reconnect routine and
handle None result the same way load_session() did), or if the repo design never
requires device lookup by id, remove the load_session_by_device_id method and
update its unit tests to use the public store.load_session() API instead.
---
Nitpick comments:
In `@src/pymax/types/events/message.py`:
- Around line 67-74: In the opcode-128 branch where you extract
message/message_id/chat_id, add a fallback to pull chatId/chat_id from the
nested message when top-level chatId is missing: after computing message (and
message_id), set chat_id = data.get("chatId", data.get("chat_id")) or
(message.get("chatId", message.get("chat_id")) if isinstance(message, dict) else
getattr(message, "chatId", getattr(message, "chat_id", None))); keep the
existing early-return check that returns data if chat_id or message_id is still
None. This ensures the branch handles payloads that only include chatId inside
message.
🪄 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: 0334e72e-6b5a-45e3-9398-04b413038158
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (84)
.github/workflows/tests.ymldocs/api/client-client.rstdocs/api/client-config.rstdocs/api/client-web.rstdocs/api/client.rstdocs/auth.rstdocs/client.rstdocs/index.rstdocs/messages.rstdocs/release-2-2-0.rstdocs/router.rstdocs/types/index.rstdocs/types/message_read_event.rstdocs/types/presence_event.rstdocs/types/reaction_update_event.rstdocs/types/typing_event.rstpyproject.tomlsrc/pymax/__init__.pysrc/pymax/api/auth/payloads.pysrc/pymax/api/auth/service.pysrc/pymax/api/binding.pysrc/pymax/api/chats/service.pysrc/pymax/api/messages/enums.pysrc/pymax/api/messages/payloads.pysrc/pymax/api/messages/service.pysrc/pymax/api/models.pysrc/pymax/api/response.pysrc/pymax/api/self/service.pysrc/pymax/api/session/payloads.pysrc/pymax/api/session/service.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/enums.pysrc/pymax/dispatch/mapping.pysrc/pymax/dispatch/resolvers.pysrc/pymax/dispatch/router.pysrc/pymax/formatting/markdown.pysrc/pymax/infra/chat.pysrc/pymax/infra/message.pysrc/pymax/logging.pysrc/pymax/protocol/tcp/compression.pysrc/pymax/protocol/tcp/framing.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/types/domain/attachments/unknown.pysrc/pymax/types/domain/auth.pysrc/pymax/types/domain/chat.pysrc/pymax/types/domain/message.pysrc/pymax/types/domain/presence.pysrc/pymax/types/domain/sync.pysrc/pymax/types/events/__init__.pysrc/pymax/types/events/mark.pysrc/pymax/types/events/message.pysrc/pymax/types/events/presence.pysrc/pymax/types/events/reaction.pysrc/pymax/types/events/typing.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.py
Comment on lines
+22
to
+23
| - name: Checkout repository | ||
| uses: actions/checkout@v6 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".github/workflows/tests.yml"
echo "== uses entries not pinned to 40-char SHA =="
rg -nP '^\s*uses:\s*[^@\s]+@(?!(?:[0-9a-fA-F]{40})\b)' "$file" || true
echo
echo "== checkout blocks (verify persist-credentials: false is present) =="
rg -nP '^\s*uses:\s*actions/checkout@' -A4 "$file" || trueRepository: MaxApiTeam/PyMax
Length of output: 596
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".github/workflows/tests.yml"
echo "== occurrences of persist-credentials =="
rg -n "persist-credentials" "$file" || true
echo
echo "== full blocks around checkout steps (lines 15-35 and 40-60) =="
nl -ba "$file" | sed -n '1,120p' | sed -n '15,60p'Repository: MaxApiTeam/PyMax
Length of output: 228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".github/workflows/tests.yml"
echo "== occurrences of persist-credentials =="
rg -n "persist-credentials" "$file" || true
echo
echo "== checkout-related region (15-35) =="
cat -n "$file" | sed -n '15,35p'
echo
echo "== checkout-related region (45-65) =="
cat -n "$file" | sed -n '45,65p'Repository: MaxApiTeam/PyMax
Length of output: 1447
Pin GitHub Actions to commit SHAs and disable checkout credential persistence.
.github/workflows/tests.ymlusesactions/checkout@v6andastral-sh/setup-uv@v8.1.0(tag-based, not full commit SHAs).- Both
actions/checkoutsteps lackwith: persist-credentials: false.
🔧 Suggested hardening diff
- - name: Checkout repository
- uses: actions/checkout@v6
+ - name: Checkout repository
+ uses: actions/checkout@<full-length-commit-sha>
+ with:
+ persist-credentials: false
@@
- - name: Install uv
- uses: astral-sh/setup-uv@v8.1.0
+ - name: Install uv
+ uses: astral-sh/setup-uv@<full-length-commit-sha>
@@
- - name: Checkout repository
- uses: actions/checkout@v6
+ - name: Checkout repository
+ uses: actions/checkout@<full-length-commit-sha>
+ with:
+ persist-credentials: false
@@
- - name: Install uv
- uses: astral-sh/setup-uv@v8.1.0
+ - name: Install uv
+ uses: astral-sh/setup-uv@<full-length-commit-sha>🧰 Tools
🪛 zizmor (1.25.2)
[warning] 22-23: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 23-23: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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/tests.yml around lines 22 - 23, Update the GitHub Actions
workflow step that currently reads "name: Checkout repository" with "uses:
actions/checkout@v6" and any other actions referenced by tag (e.g.,
"astral-sh/setup-uv@v8.1.0") to use specific commit SHAs instead of tags, and
add "with: persist-credentials: false" under each actions/checkout step to
disable credential persistence; ensure you replace the tag references with the
corresponding full commit SHA for actions/checkout and astral-sh/setup-uv and
add the persist-credentials setting for every checkout step in the workflow.
Source: Linters/SAST tools
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.2.0.
read_message()теперь сохраняется.Тип изменений
Связанные задачи / Issue
#62
Summary by CodeRabbit
Release Notes: PyMax 2.2.0
New Features
Bug Fixes
Documentation