chore: merge dev to master #28
Conversation
Switch docker-compose to use the prebuilt aethersailor/rule-bot image and add a commented ADMIN_USER_IDS environment option (supports multiple Telegram IDs, lets admins force-add domains and access debug info). Update README quickstart to show downloading the compose file, editing required env vars (TELEGRAM_BOT_TOKEN, GITHUB_TOKEN, GITHUB_REPO, DIRECT_RULE_FILE), and document the ADMIN_USER_IDS admin mode. Small doc/formatting tweaks to logs/FAQ for clarity.
There was a problem hiding this comment.
Pull request overview
This PR merges the dev branch into master, introducing admin force-add functionality and UX improvements. The changes enable administrators to bypass domain quality checks and add domains that would normally be rejected, while also improving the user experience by maintaining state persistence across operations.
Changes:
- Added admin force-add feature allowing designated users to bypass domain quality checks for rejected domains
- Implemented
/idcommand to help users discover their Telegram user IDs - Modified state management to keep users in query/add modes after operations for improved workflow continuity
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/config.py | Added ADMIN_USER_IDS configuration and parsing logic for admin user management |
| src/services/github_service.py | Added force_add parameter to distinguish admin-forced additions in commit messages and comments |
| src/handlers/handler_manager.py | Implemented admin permission checks, force-add handler, /id command, state persistence improvements, and markdown escaping fixes |
| src/handlers/group_handler.py | Added admin force-add button for rejected domains in group chat mode |
| src/bot.py | Registered /id command handler |
| docker-compose.yml | Changed from build to image deployment and added ADMIN_USER_IDS environment variable documentation |
| README.md | Updated quick start guide and documented admin mode functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """处理管理员权限强制添加回调""" | ||
| try: | ||
| if not self.is_admin(user_id): | ||
| logger.warning(f"管理员权限操作被拒绝: user_id={user_id}, data={data}") |
There was a problem hiding this comment.
The admin permission check only logs a warning but doesn't provide user feedback when a non-admin attempts to use the admin force-add callback. After the warning is logged, the function silently returns without notifying the user. Consider adding an error message like: await query.edit_message_text("❌ 您没有管理员权限执行此操作。")
| logger.warning(f"管理员权限操作被拒绝: user_id={user_id}, data={data}") | |
| logger.warning(f"管理员权限操作被拒绝: user_id={user_id}, data={data}") | |
| await query.edit_message_text("❌ 您没有管理员权限执行此操作。") |
| result_text += f"📍 **域名：** `{target_domain}`\n" | ||
| result_text += f"👤 **提交者：** @{self.escape_markdown(username)}\n" | ||
| if description: | ||
| result_text += f"📝 **说明：** {self.escape_markdown(description)}\n" | ||
| result_text += f"📂 **文件路径：** `{self.escape_markdown(add_result['file_path'])}`\n" | ||
| result_text += f"📂 **文件路径：** `{add_result['file_path']}`\n" | ||
| if add_result.get('commit_url'): | ||
| result_text += f"🔗 **查看提交：** [点击查看]({add_result['commit_url']})\n" | ||
| result_text += f"📝 **Commit ID：** `{add_result.get('commit_sha', '')[:8]}`\n" | ||
| result_text += f"💬 **提交信息：** `{self.escape_markdown(add_result['commit_message'])}`\n" | ||
| result_text += f"💬 **提交信息：** `{add_result['commit_message']}`\n" | ||
| result_text += f"\n💡 本小时内还可添加 {remaining} 个域名" | ||
| else: | ||
| result_text = f"❌ **域名添加失败**\n\n" | ||
| result_text += f"📍 **域名：** `{self.escape_markdown(target_domain)}`\n" | ||
| result_text += f"📍 **域名：** `{target_domain}`\n" | ||
| result_text += f"❌ **错误：** {self.escape_markdown(add_result.get('error', '未知错误'))}" |
There was a problem hiding this comment.
The same markdown escaping issue exists here. Domain, file_path, and commit_message are displayed without escaping (lines 1371, 1375, 1379, 1383). Since commit_message can contain user-provided description text, this could potentially cause Markdown rendering issues if special characters are present.
| # 防重复检查 | ||
| github_result = await self.github_service.check_domain_in_rules(domain) | ||
| if github_result.get("exists"): | ||
| result_text = f"❌ **域名已存在于规则中**\n\n" | ||
| result_text += f"📍 **域名：** `{domain}`\n\n" | ||
| result_text += "📋 **找到的规则：**\n" | ||
| for match in github_result.get("matches", []): | ||
| result_text += f" • 第{match['line']}行: {match['rule']}\n" | ||
|
|
||
| keyboard = [ | ||
| [InlineKeyboardButton("➕ 添加其他域名", callback_data="add_direct_rule")], | ||
| [InlineKeyboardButton("🏠 返回主菜单", callback_data="main_menu")] | ||
| ] | ||
| reply_markup = InlineKeyboardMarkup(keyboard) | ||
| await query.edit_message_text(result_text, reply_markup=reply_markup, parse_mode='Markdown') | ||
| return | ||
|
|
||
| in_geosite = await self.data_manager.is_domain_in_geosite(domain) | ||
| if in_geosite: | ||
| result_text = f"❌ **域名已存在于 GEOSITE:CN 中**\n\n" | ||
| result_text += f"📍 **域名：** `{domain}`\n\n" | ||
| result_text += "该域名已在 GEOSITE:CN 规则中，不需要重复添加。" | ||
|
|
||
| keyboard = [ | ||
| [InlineKeyboardButton("➕ 添加其他域名", callback_data="add_direct_rule")], | ||
| [InlineKeyboardButton("🏠 返回主菜单", callback_data="main_menu")] | ||
| ] | ||
| reply_markup = InlineKeyboardMarkup(keyboard) | ||
| await query.edit_message_text(result_text, reply_markup=reply_markup, parse_mode='Markdown') | ||
| return |
There was a problem hiding this comment.
The admin force-add handler still performs duplicate domain checks (GitHub rules and GEOSITE:CN checks at lines 1103-1131) before adding. This is good for safety, but it means that the "force add" feature doesn't actually bypass these checks. The only checks being bypassed are the domain quality checks (DNS/GeoIP). Consider either: (1) documenting this behavior clearly (that force-add only bypasses quality checks, not duplicate checks), or (2) adding an option to truly force-add even if duplicates exist, if that's the intended use case.
| def is_admin(self, user_id: int) -> bool: | ||
| """检查是否管理员""" | ||
| return user_id in self.config.ADMIN_USER_IDS | ||
|
|
||
| def get_admin_force_add_callback(self, domain: str) -> str: | ||
| """构建管理员权限添加的回调数据""" | ||
| return f"admin_force_add|{domain}" |
There was a problem hiding this comment.
The new admin force-add functionality (including is_admin, get_admin_force_add_callback, and _handle_admin_force_add_callback methods) lacks test coverage. The repository has existing test files (test_async_services.py, test_domain_checker.py, test_domain_utils.py) that demonstrate comprehensive testing patterns. Consider adding tests for: (1) is_admin method with valid/invalid user IDs, (2) get_admin_force_add_callback format validation, (3) _handle_admin_force_add_callback permission checks and success/failure scenarios, (4) force_add parameter in GitHubService.add_domain_to_rules.
| if should_reject: | ||
| self.set_user_state(user_id, "waiting_add_domain") |
There was a problem hiding this comment.
The user state is set to "waiting_add_domain" only when the domain is rejected (line 994-995). However, when the domain is accepted (line 968-971), the state is not explicitly updated. This creates inconsistent behavior - rejected domains keep the user in add mode, but accepted domains leave the user in whatever state they were in previously. This could potentially cause issues if the user state isn't properly managed in the confirm callback. Consider also setting the state for non-rejected cases for consistency, or document why this asymmetric behavior is intentional.
| if should_reject: | |
| self.set_user_state(user_id, "waiting_add_domain") | |
| self.set_user_state(user_id, "waiting_add_domain") |
| 配置 `ADMIN_USER_IDS` 后，指定的管理员用户可以： | ||
|
|
||
| - 强制添加被系统检测拒绝的域名 | ||
| - 获取调试辅助信息 |
There was a problem hiding this comment.
The README documentation mentions that admins can "获取调试辅助信息" (get debugging assistance information) at line 156, but this feature does not appear to be implemented in the codebase. Only the force-add functionality is implemented for admins. Either implement the debugging feature or remove this claim from the documentation to avoid confusion.
| 1) 创建工作目录并下载配置文件 | ||
|
|
||
| ```bash | ||
| mkdir -p /opt/Rule-Bot && cd /opt/Rule-Bot | ||
| wget https://raw.githubusercontent.com/Aethersailor/Rule-Bot/main/docker-compose.yml | ||
| ``` | ||
|
|
||
| 2) 启动 | ||
| 1) 编辑配置文件 | ||
|
|
||
| ```bash | ||
| vim docker-compose.yml | ||
| ``` | ||
|
|
||
| 修改以下必填参数（去掉 `#` 注释，填入你的实际值）： | ||
|
|
||
| - `TELEGRAM_BOT_TOKEN` | ||
| - `GITHUB_TOKEN` | ||
| - `GITHUB_REPO` | ||
| - `DIRECT_RULE_FILE` | ||
|
|
||
| 1) 启动容器 | ||
|
|
||
| ```bash | ||
| docker compose up -d | ||
| ``` | ||
|
|
||
| 3) 查看日志 | ||
| 1) 查看日志 |
There was a problem hiding this comment.
The numbering in the Quick Start section is incorrect. All steps are numbered as "1)" instead of being sequentially numbered (1, 2, 3, 4). This makes the documentation confusing to follow. The steps should be numbered: 1) 创建工作目录并下载配置文件, 2) 编辑配置文件, 3) 启动容器, 4) 查看日志
| # 检查用户添加频率限制 | ||
| can_add, _ = self.check_user_add_limit(user_id) | ||
| if not can_add: | ||
| keyboard = [ | ||
| [InlineKeyboardButton("🔍 查询域名", callback_data="query_domain")], | ||
| [InlineKeyboardButton("🏠 返回主菜单", callback_data="main_menu")] | ||
| ] | ||
| reply_markup = InlineKeyboardMarkup(keyboard) | ||
| await query.edit_message_text( | ||
| "⚠️ **添加频率限制**\n\n" | ||
| f"您在当前小时内已达到添加上限（{self.MAX_ADDS_PER_HOUR}个域名）。\n\n" | ||
| "🕐 请等待一小时后再尝试添加新域名。\n\n" | ||
| "💡 此限制是为了防止系统滥用，感谢您的理解。", | ||
| reply_markup=reply_markup, | ||
| parse_mode='Markdown' | ||
| ) | ||
| return |
There was a problem hiding this comment.
Admin users can bypass rate limiting when using force-add functionality. The check_user_add_limit method enforces rate limits even for admins (lines 1085-1100), but admins should arguably have higher or no rate limits since they have elevated privileges. Consider either: (1) removing rate limit checks for admins in force-add scenarios, (2) implementing separate, higher rate limits for admins, or (3) documenting why admins are intentionally rate-limited.
| result_text += f"📍 **域名：** `{target_domain}`\n" | ||
| result_text += f"👤 **提交者：** @{self.escape_markdown(username)}\n" | ||
| if description: | ||
| result_text += f"📝 **说明：** {self.escape_markdown(description)}\n" | ||
| result_text += f"📂 **文件路径：** `{self.escape_markdown(add_result['file_path'])}`\n" | ||
| result_text += f"📂 **文件路径：** `{add_result['file_path']}`\n" | ||
| if add_result.get('commit_url'): | ||
| result_text += f"🔗 **查看提交：** [点击查看]({add_result['commit_url']})\n" | ||
| result_text += f"📝 **Commit ID：** `{add_result.get('commit_sha', '')[:8]}`\n" | ||
| result_text += f"💬 **提交信息：** `{self.escape_markdown(add_result['commit_message'])}`\n" | ||
| result_text += f"💬 **提交信息：** `{add_result['commit_message']}`\n" | ||
| result_text += f"\n💡 本小时内还可添加 {remaining} 个域名" | ||
| else: | ||
| result_text = f"❌ **域名添加失败**\n\n" | ||
| result_text += f"📍 **域名：** `{self.escape_markdown(target_domain)}`\n" | ||
| result_text += f"📍 **域名：** `{target_domain}`\n" | ||
| result_text += f"❌ **错误：** {self.escape_markdown(add_result.get('error', '未知错误'))}" |
There was a problem hiding this comment.
Markdown escaping was removed from domain, file_path, and commit_message fields (lines 1306, 1310, 1314, 1318) which may contain special characters. While domains and file paths might not typically contain Markdown special characters, commit messages could contain user-supplied description text that may include special characters. This creates an inconsistency where some user-provided text is escaped (username, description, errors) while commit_message is not. Consider whether commit_message should also be escaped, especially since it can include user-provided description text.
| def _parse_user_ids(self, ids_str: str) -> list: | ||
| """解析用户 ID 列表 | ||
|
|
||
| Args: | ||
| ids_str: 逗号分隔的用户 ID 字符串 | ||
|
|
||
| Returns: | ||
| 用户 ID 整数列表 | ||
| """ | ||
| if not ids_str.strip(): | ||
| return [] | ||
|
|
||
| user_ids = [] | ||
| parts = [part for part in re.split(r"[,\s;]+", ids_str.strip()) if part] | ||
| for raw_id in parts: | ||
| if not raw_id: | ||
| continue | ||
| try: | ||
| user_ids.append(int(raw_id)) | ||
| except ValueError: | ||
| logger.warning(f"无效的 ADMIN_USER_IDS: {raw_id}") | ||
| return user_ids |
There was a problem hiding this comment.
The _parse_user_ids method uses a more sophisticated regex-based split pattern (re.split with multiple separators including commas, whitespace, and semicolons) compared to _parse_group_ids which only splits on commas. This creates an inconsistency in how the two similar configuration options are parsed. While the extra flexibility for user IDs might be intentional, it would be better to either: (1) make both methods use the same parsing logic for consistency, or (2) document why user IDs support more separator types than group IDs.
No description provided.