Dev/2.1.2#56
Conversation
📝 WalkthroughWalkthroughPyMax 2.1.2 release updates the package version, makes ChangesPyMax 2.1.2 Release
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.github/workflows/publish.yml (1)
24-30: ⚡ Quick winPin the uv CLI version explicitly.
Both jobs install
uv, but neithersetup-uvstep sets aversion/version-file, and this repo’spyproject.tomldoes not provide a required uv version. Per the action docs, that falls back to the latest uv release, so a future upstream uv change can alter or break the release pipeline without any change in this repo. (github.com)Also applies to: 60-64
🤖 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 24 - 30, The workflow's "Install uv" steps use astral-sh/setup-uv without pinning a uv CLI version, so add an explicit pin by updating the action inputs for the astral-sh/setup-uv steps (the steps named "Install uv") to include either a fixed with: version: "<UV_VERSION>" or point to a version-file (with: version-file: "pyproject.toml"); do this for both occurrences of the Install uv step so the release pipeline doesn't float to the latest upstream uv automatically.src/pymax/api/bots/service.py (1)
23-30: ⚡ Quick winAdd a regression test for the omitted
chat_idpath.This public API now supports
get_init_data(bot_id, start_param=...), but the provided test coverage only asserts the serialized payload whenchat_idis present. Please add a case that verifieschatIdis omitted from the outgoing payload whenchat_id=None, so this release behavior stays locked down.🤖 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/bots/service.py` around lines 23 - 30, Add a regression test that calls the public API method get_init_data(bot_id, start_param=...) (or via the service wrapper that calls RequestInitDataPayload) with chat_id omitted and asserts the outgoing payload passed to self.app.invoke (Opcode.WEB_APP_INIT_DATA) does not include a chatId field; mock or spy on service.app.invoke, capture its payload argument and verify RequestInitDataPayload.to_payload() for the path where chat_id is None omits chatId (use get_init_data and RequestInitDataPayload as the entrypoints to locate code).tests/app/test_app_runtime.py (1)
155-170: ⚡ Quick winAdd the matching regression test for tokenless login responses.
This file now covers the timeout fix, but the other runtime change in this PR—keeping the existing session token when
LOGINreturnstoken=None—is still untested. A startup test that returns a login payload without a replacement token would lock in the new behavior and catch regressions in Lines 130-132 ofsrc/pymax/app.py.🤖 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/app/test_app_runtime.py` around lines 155 - 170, Add a regression test that verifies when the app receives a LOGIN response with token=None the existing session token is preserved: create a RuntimeStore with SessionInfo(token="token", ...), construct a RuntimeConnection whose startup frames include a LOGIN response whose payload explicitly has token=None, instantiate App (or call the startup method used in the runtime flow) with that store and connection, run the startup/invoke sequence that processes LOGIN, then assert the store.session.token remains "token" (unchanged) and that the connection processed the LOGIN frame; reference runtime symbols RuntimeStore, SessionInfo, RuntimeConnection, App, and the LOGIN response handling to locate the code paths under test.
🤖 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 21-25: Replace all movable tag references in the workflow with
full commit SHAs: update uses: for actions/checkout@v6,
astral-sh/setup-uv@v8.1.0 (both occurrences), actions/upload-artifact@v4, and
actions/download-artifact@v4 to their corresponding full-length commit SHAs;
ensure the OIDC "publish" job's id-token: write step also points to the
SHA-pinned actions. Locate these by their uses: lines (e.g., "uses:
actions/checkout", "uses: astral-sh/setup-uv", "uses: actions/upload-artifact",
"uses: actions/download-artifact") and replace the tag suffix (e.g., `@v6`) with
the exact commit SHA obtained from each action's repo.
In `@src/pymax/app.py`:
- Line 36: The constructor currently treats any falsy config.store as missing by
using "self.config.store or SessionStore(...)", which overwrites intentionally
injected falsy stores; change the logic to an explicit None check so injected
stores are preserved: set self.store to config.store if config.store is not
None, otherwise construct a new SessionStore using SessionStore(config.work_dir,
config.session_name); update the assignment for self.store in the initializer
where self.config.store and SessionStore are referenced.
---
Nitpick comments:
In @.github/workflows/publish.yml:
- Around line 24-30: The workflow's "Install uv" steps use astral-sh/setup-uv
without pinning a uv CLI version, so add an explicit pin by updating the action
inputs for the astral-sh/setup-uv steps (the steps named "Install uv") to
include either a fixed with: version: "<UV_VERSION>" or point to a version-file
(with: version-file: "pyproject.toml"); do this for both occurrences of the
Install uv step so the release pipeline doesn't float to the latest upstream uv
automatically.
In `@src/pymax/api/bots/service.py`:
- Around line 23-30: Add a regression test that calls the public API method
get_init_data(bot_id, start_param=...) (or via the service wrapper that calls
RequestInitDataPayload) with chat_id omitted and asserts the outgoing payload
passed to self.app.invoke (Opcode.WEB_APP_INIT_DATA) does not include a chatId
field; mock or spy on service.app.invoke, capture its payload argument and
verify RequestInitDataPayload.to_payload() for the path where chat_id is None
omits chatId (use get_init_data and RequestInitDataPayload as the entrypoints to
locate code).
In `@tests/app/test_app_runtime.py`:
- Around line 155-170: Add a regression test that verifies when the app receives
a LOGIN response with token=None the existing session token is preserved: create
a RuntimeStore with SessionInfo(token="token", ...), construct a
RuntimeConnection whose startup frames include a LOGIN response whose payload
explicitly has token=None, instantiate App (or call the startup method used in
the runtime flow) with that store and connection, run the startup/invoke
sequence that processes LOGIN, then assert the store.session.token remains
"token" (unchanged) and that the connection processed the LOGIN frame; reference
runtime symbols RuntimeStore, SessionInfo, RuntimeConnection, App, and the LOGIN
response handling to locate the code paths under test.
🪄 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: fb2c549c-b4f2-4991-b2fa-75e70edcdd9c
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/publish.ymldocs/index.rstdocs/release-2-1-2.rstpyproject.tomlsrc/pymax/__init__.pysrc/pymax/api/bots/payloads.pysrc/pymax/api/bots/service.pysrc/pymax/app.pysrc/pymax/types/domain/folder.pysrc/pymax/types/domain/login.pytests/api/test_chat_user_self_session_services.pytests/app/test_app_runtime.py
💤 Files with no reviewable changes (1)
- src/pymax/types/domain/folder.py
| - name: Checkout repository | ||
| uses: actions/checkout@v6 | ||
|
|
||
| - name: Set up uv | ||
| uses: astral-sh/setup-uv@v4 | ||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v8.1.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and show the publish workflow with line numbers
FILE=".github/workflows/publish.yml"
if [ ! -f "$FILE" ]; then
echo "Missing: $FILE"
exit 1
fi
echo "---- $FILE (with line numbers) ----"
nl -ba "$FILE" | sed -n '1,200p'
echo "---- End ----"
# Extract all `uses:` lines for quick verification
echo "---- uses: lines ----"
rg -n "^\s*uses:\s*" "$FILE" || trueRepository: MaxApiTeam/PyMax
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/publish.yml"
if [ ! -f "$FILE" ]; then
echo "Missing: $FILE"
exit 1
fi
echo "---- $FILE (with line numbers) ----"
awk '{printf "%d\t%s\n", NR, $0}' "$FILE" | sed -n '1,200p'
echo "---- uses: lines ----"
rg -n "^\s*uses:\s*" "$FILE" || trueRepository: MaxApiTeam/PyMax
Length of output: 1772
🌐 Web query:
GitHub Actions security hardening pin actions to full commit SHA immutable only form movable tags
💡 Result:
Pin GitHub Actions by full-length commit SHA (immutable), not by tags (movable). GitHub’s own guidance says pinning to a full-length commit SHA is currently the only way to treat an action reference as an immutable release, and warns that specifying a tag carries risk because tags can be moved or deleted if an attacker gains access to the action’s repository.[1][2] In a workflow, use this form: - uses: owner/repo@ For example, the docs’ guidance is that the reference should be a full-length commit SHA; the short/abbreviated SHA form is explicitly discouraged as insecure.[3] Also consider enforcing this via GitHub Actions policy: - GitHub introduced an “allowed actions policy” capability to enforce SHA pinning; workflows that attempt to use an action that isn’t pinned to a full commit SHA will fail the check.[4] Why “full commit SHA only” matters (vs tags): - GitHub explains that SHA pinning helps mitigate backdoors because an attacker would need to generate a SHA-1 collision for a valid Git object payload.[1] - GitHub also notes that pinning to tags is less secure and depends on trusting the creator; even then, tags can be moved/deleted.[1][2] Practical hardening tips (beyond pinning): - Audit the action’s source code and behavior with respect to how it handles repository contents and secrets.[2][5]
Citations:
- 1: https://docs.github.com/en/actions/reference/security/secure-use
- 2: https://github.com/github/docs/blob/962a1c8dccb8c0f66548b324e5b921b5e4fbc3d6/content/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions.md
- 3: https://docs.github.com/en/enterprise-server@2.22/actions/security-guides/security-hardening-for-github-actions
- 4: https://github.blog/changelog/2025-08-15-github-actions-policy-now-supports-blocking-and-sha-pinning-actions/
- 5: https://docs.github.com/en/enterprise-server@3.9/actions/security-guides/security-hardening-for-github-actions
Pin all GitHub Actions uses: references to full commit SHAs in .github/workflows/publish.yml (including the OIDC publish job).
This workflow trusts movable tags for:
actions/checkout@v6(line 22)astral-sh/setup-uv@v8.1.0(lines 25, 61)actions/upload-artifact@v4(line 36)actions/download-artifact@v4(line 55)
Because the publish job has id-token: write, a retagged action could tamper with what uv publish publishes. GitHub guidance recommends full-length commit SHAs for immutable action references. https://docs.github.com/en/actions/reference/security/secure-use
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 21-22: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 22-22: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 25-25: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 25-25: 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 21 - 25, Replace all movable tag
references in the workflow with full commit SHAs: update uses: for
actions/checkout@v6, astral-sh/setup-uv@v8.1.0 (both occurrences),
actions/upload-artifact@v4, and actions/download-artifact@v4 to their
corresponding full-length commit SHAs; ensure the OIDC "publish" job's id-token:
write step also points to the SHA-pinned actions. Locate these by their uses:
lines (e.g., "uses: actions/checkout", "uses: astral-sh/setup-uv", "uses:
actions/upload-artifact", "uses: actions/download-artifact") and replace the tag
suffix (e.g., `@v6`) with the exact commit SHA obtained from each action's repo.
| self.store = self.config.store or SessionStore( | ||
| config.work_dir, config.session_name | ||
| ) | ||
| self.store = self.config.store or SessionStore(config.work_dir, config.session_name) |
There was a problem hiding this comment.
Preserve injected stores even when they are falsy.
Line 36 now treats any falsy custom store as “missing” and replaces it with a new SessionStore. That breaks the config.store injection contract and can silently switch callers from their own persistence backend to on-disk storage. Use an explicit None check here instead.
Suggested fix
- self.store = self.config.store or SessionStore(config.work_dir, config.session_name)
+ self.store = (
+ self.config.store
+ if self.config.store is not None
+ else SessionStore(config.work_dir, config.session_name)
+ )🤖 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/app.py` at line 36, The constructor currently treats any falsy
config.store as missing by using "self.config.store or SessionStore(...)", which
overwrites intentionally injected falsy stores; change the logic to an explicit
None check so injected stores are preserved: set self.store to config.store if
config.store is not None, otherwise construct a new SessionStore using
SessionStore(config.work_dir, config.session_name); update the assignment for
self.store in the initializer where self.config.store and SessionStore are
referenced.
1 participant
Описание
Кратко, что делает этот PR. Например, добавляет новый метод, исправляет баг, улучшает документацию.
Подготавливает релиз 2.1.2: исправляет обработку timeout и login-ответов без нового токена, убирает проблемный iterator у FolderList, обновляет publish workflow, bump версии и release notes.
Тип изменений
Связанные задачи / Issue
Ссылка на issue, если есть: #
#55
Summary by CodeRabbit
New Features
get_bot_init_data()) now works without specifying a chat ID for web-app scenarios.Bug Fixes
Chores