Fix/p0 p3 code review fixes - #552
Conversation
- _download_single_image: os.path.exists -> file_exists (from common) - _decode_pool: decode_worker 默认 min(4, cpu_count or 1)，防无上限创建线程
- jm_config: DEFAULT_OPTION_DICT['download']['threading'] 新增 decode_worker 字段 - jm_config: option_default_dict() 中 decode_worker 默认 min(4, cpu_count or 1) - jm_async_downloader: decode_worker 从 option.download.threading.decode_worker 回退 - 移除 jm_async_downloader 中已无用的 import os
- P0-1: zip_with_password 使用 shlex.quote() 防止 shell 命令注入 - P0-2: execute_cmd 从 os.system 改为 subprocess.run，添加注入警告文档 - P0-3: execute_deletion 添加路径包含校验，只删除 base_dir 范围内的文件 - P0-4: decide_image_save_dir 添加 realpath 校验，防止路径遍历逃逸 base_dir
v2.7.2: 更新JM内置域名; 废弃失效的 GitHub 域名抓取实现; 完善 async API，新增 categories_fil…
docs: 新增used by，star history图，更新 repo card 链接。 (#550)
P0 (安全/稳定): - jm_plugin.py: execute_cmd/execute_multi_line_cmd 支持列表参数避免 shell 注入; zip_with_password 改用 subprocess 列表形式 + cwd - jm_client_impl.py: FutureWrapper.result() 用 try/finally 确保异常时 done 标志被设置 - jm_option.py: call_all_plugin 捕获 KeyboardInterrupt/SystemExit 改为直接 re-raise - jm_exception.py + jm_config.py: 将 format_album_url 下沉到 jm_config 消除循环依赖 - jm_client_impl.py: JSON 检查限制前 1024 字符; resp.request.url 加防御性 getattr P1 (可维护性): - jm_toolkit.py: limit_text 补上缺失的闭合括号 - jm_client_impl.py + jm_config.py: SCRAMBLE_CACHE 添加 threading.Lock 双重检查锁定 P2 (代码整洁): - api.py: 提取 _download_and_return/_download_async_and_return 消除 sync/async 重复 - pyproject.toml: 添加 curl-cffi/pillow/pycryptodome/pyyaml 版本下限; 声明可选依赖 P3 (小改进): - jm_client_impl.py: 删除空的 get_username_from_cookies 方法 - cli.py: 空参数时打印使用提示 - README.md: 移除过时版本号 (1.6.3) - jm_option.py: float 版本比较改为 tuple 比较
📝 WalkthroughWalkthroughThe PR centralizes synchronous and asynchronous download handling, adds configuration defaults and cache synchronization, strengthens path and command safety, updates shared URL and client utilities, improves CLI handling, and adjusts project dependencies and documentation. ChangesCore runtime updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant DownloadAPI
participant Downloader
participant Callback
Caller->>DownloadAPI: request album or photo download
DownloadAPI->>Downloader: create context and add features
DownloadAPI->>Downloader: download entity
DownloadAPI->>Callback: invoke optional callback
DownloadAPI->>Caller: return DownloadResult
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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: 4
🤖 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/jmcomic/api.py`:
- Around line 81-88: Update the docstrings around the batch-download API and the
additional referenced sections to avoid RUF002 violations from fullwidth
punctuation. Replace the newly introduced fullwidth punctuation with ASCII
punctuation while preserving the Chinese documentation meaning; do not alter the
API behavior.
In `@src/jmcomic/cli.py`:
- Around line 105-107: Move the empty-ID guard in main() to immediately after
parse_arg() returns, before constructing/loading the option or invoking
after_init plugins. Preserve the existing message and early return, using
album_id_list and photo_id_list from the parsed arguments.
In `@src/jmcomic/jm_client_impl.py`:
- Around line 1107-1113: Move the self.after_done_callback() invocation into the
existing finally block surrounding self.future.result() in the wrapper
completion logic. Ensure it runs after setting self.done and clearing
self.future regardless of whether result() succeeds or raises, so failed cached
wrappers are removed consistently.
In `@src/jmcomic/jm_option.py`:
- Around line 260-266: Replace the separator-based containment check in the
image-directory validation around resolved and base in src/jmcomic/jm_option.py
lines 260-266 with an os.path.commonpath([base, resolved]) comparison against
base. Apply the same containment change in the deletion-target guard in
src/jmcomic/jm_plugin.py lines 84-88, preserving rejection or skipping of paths
outside the base directory while allowing valid paths under filesystem or drive
roots.
🪄 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: 194b4b68-e49d-4922-87c4-a0c26766d8ef
📒 Files selected for processing (11)
README.mdpyproject.tomlsrc/jmcomic/api.pysrc/jmcomic/cli.pysrc/jmcomic/jm_async_downloader.pysrc/jmcomic/jm_client_impl.pysrc/jmcomic/jm_config.pysrc/jmcomic/jm_exception.pysrc/jmcomic/jm_option.pysrc/jmcomic/jm_plugin.pysrc/jmcomic/jm_toolkit.py
| 当jm_album_id不是str或int时，视为批量下载 | ||
| :param jm_album_id: 本子的禁漫车号 | ||
| :param option: 下载选项 | ||
| :param downloader: 下载器类 | ||
| :param callback: 返回值回调函数，可以拿到 album 和 downloader | ||
| :param check_exception: 是否检查异常, 如果为True，会检查downloader是否有下载异常，并上抛PartialDownloadFailedException | ||
| :param extra: 下载特性（Feature），下载时动态挂载的附加行为上下文。会自动根据上下文（如 album/photo 来源）自适应参数行为。支持单个 Feature、FeatureChain、或列表 | ||
| :return: 对于的本子实体类，下载器（如果是上述的批量情况，返回值为download_batch的返回值） | ||
| :param check_exception: 是否检查异常 | ||
| :param extra: 下载特性（Feature） | ||
| :return: DownloadResult (如果是批量情况返回 BatchResult) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid introducing RUF002 violations in these docstrings.
The newly added fullwidth punctuation is flagged by Ruff and can fail lint when RUF002 is enabled. Replace it with ASCII punctuation or deliberately suppress/configure this rule for Chinese documentation.
Also applies to: 107-114, 190-192, 216-218
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 81-81: Docstring contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF002)
[warning] 85-85: Docstring contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF002)
[warning] 87-87: Docstring contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF002)
[warning] 87-87: Docstring contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF002)
🤖 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/jmcomic/api.py` around lines 81 - 88, Update the docstrings around the
batch-download API and the additional referenced sections to avoid RUF002
violations from fullwidth punctuation. Replace the newly introduced fullwidth
punctuation with ASCII punctuation while preserving the Chinese documentation
meaning; do not alter the API behavior.
Source: Linters/SAST tools
| if len(self.album_id_list) == 0 and len(self.photo_id_list) == 0: | ||
| print('未指定任何 id，请提供 album 或 photo 的 id，例如: jmcomic 123') | ||
| return |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exit before constructing the option.
This guard runs after main() loads the option and invokes after_init plugins. Move it immediately after parse_arg() so jmcomic with no IDs cannot trigger option/plugin side effects or failures.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 106-106: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
[warning] 106-106: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
🤖 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/jmcomic/cli.py` around lines 105 - 107, Move the empty-ID guard in main()
to immediately after parse_arg() returns, before constructing/loading the option
or invoking after_init plugins. Preserve the existing message and early return,
using album_id_list and photo_id_list from the parsed arguments.
| try: | ||
| result = self.future.result() | ||
| self._result = result | ||
| finally: | ||
| self.done = True | ||
| self.future = None # help gc | ||
| self.after_done_callback() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Run the cache-removal callback in the finally block.
If future.result() raises, Line 1113 is skipped. The failed wrapper stays cached as done=True, so later callers return None instead of retrying or receiving the failure.
Proposed fix
def result(self):
if not self.done:
try:
result = self.future.result()
self._result = result
finally:
self.done = True
self.future = None # help gc
- self.after_done_callback()
+ self.after_done_callback()
return self._result📝 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.
| try: | |
| result = self.future.result() | |
| self._result = result | |
| finally: | |
| self.done = True | |
| self.future = None # help gc | |
| self.after_done_callback() | |
| try: | |
| result = self.future.result() | |
| self._result = result | |
| finally: | |
| self.done = True | |
| self.future = None # help gc | |
| self.after_done_callback() |
🤖 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/jmcomic/jm_client_impl.py` around lines 1107 - 1113, Move the
self.after_done_callback() invocation into the existing finally block
surrounding self.future.result() in the wrapper completion logic. Ensure it runs
after setting self.done and clearing self.future regardless of whether result()
succeeds or raises, so failed cached wrappers are removed consistently.
| # 路径遍历防护：确保解析后的路径仍在 base_dir 范围内 | ||
| resolved = os.path.realpath(save_dir) | ||
| base = os.path.realpath(self.dir_rule.base_dir) | ||
| if not resolved.startswith(base + os.sep) and resolved != base: | ||
| raise JmcomicException( | ||
| f'路径安全校验失败: {save_dir} 不在 base_dir ({base}) 范围内' | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle filesystem-root base directories in both containment checks.
base + os.sep becomes // for / (and similarly doubles separators for drive roots), so valid paths below a root base are rejected or skipped. Use os.path.commonpath() for containment instead.
src/jmcomic/jm_option.py#L260-L266: compareos.path.commonpath([base, resolved])withbasebefore rejecting an image directory.src/jmcomic/jm_plugin.py#L84-L88: apply the same comparison before skipping a deletion target.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 260-260: Comment contains ambiguous ： (FULLWIDTH COLON). Did you mean : (COLON)?
(RUF003)
📍 Affects 2 files
src/jmcomic/jm_option.py#L260-L266(this comment)src/jmcomic/jm_plugin.py#L84-L88
🤖 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/jmcomic/jm_option.py` around lines 260 - 266, Replace the separator-based
containment check in the image-directory validation around resolved and base in
src/jmcomic/jm_option.py lines 260-266 with an os.path.commonpath([base,
resolved]) comparison against base. Apply the same containment change in the
deletion-target guard in src/jmcomic/jm_plugin.py lines 84-88, preserving
rejection or skipping of paths outside the base directory while allowing valid
paths under filesystem or drive roots.
|
先去读 Contributing 文档，别把一堆ai生成的垃圾pr上来 |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation