Reorder admin permission check in execute and fix empty pattern string blocking all commands

[NancalaStarry]

调整execute中优先检查管理员权限，调整pattern为空字符串时默认不禁止所有指令

[qodo-code-review]

Review Summary by Qodo

Fix admin permission check order and empty pattern handling

🐞 Bug fix ✨ Enhancement

Walkthroughs

Description

• Reorder admin permission checks to execute before command extraction
• Fix empty pattern strings blocking all commands in ignore list
• Consolidate permission validation for execute and mcdr commands
• Improve config documentation for ignore patterns behavior

Diagram

flowchart LR
  A["Command received"] --> B["Check if execute/mcdr command"]
  B --> C["Validate admin permission early"]
  C --> D{Permission granted?}
  D -->|No| E["Return False"]
  D -->|Yes| F["Extract command content"]
  F --> G["Check ignore patterns"]
  G --> H{Pattern empty?}
  H -->|Yes| I["Skip pattern"]
  H -->|No| J["Match against pattern"]
  J --> K["Execute command"]

Loading

File Changes

1. GUGUbot/gugubot/logic/system/execute.py 🐞 Bug fix +12/-8

Reorder permission checks and fix empty patterns

• Added check to skip empty pattern strings in ignore patterns loop
• Moved admin permission validation to execute before command extraction
• Moved admin permission validation to mcdr command before command extraction
• Removed duplicate permission checks after bridge command handling

GUGUbot/gugubot/logic/system/execute.py

---

2. default_config.yml 📝 Documentation +1/-1

Improve ignore patterns configuration documentation

• Updated comment for ignore_execute_command_patterns to clarify empty pattern behavior
• Clarified that empty patterns do not ignore all commands

default_config.yml

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. #execute denial not logged 📘 Rule violation ⛨ Security

Description

Unauthorized #execute attempts return False without an audit log containing user ID, action, and
outcome. This prevents reconstructing who attempted command execution and whether it was denied.

Code

GUGUbot/gugubot/logic/system/execute.py[R127-130]

+            # 权限检查：必须是管理员才能执行命令
+            if not self.is_command(broadcast_info) or \
+                    (not await self._is_admin(broadcast_info.sender_id) and not broadcast_info.is_admin):
+                return False

Evidence

Compliance requires audit trails for critical actions; command execution attempts are
security-relevant events. The new permission-check branch denies execution via an early `return
False` with no logging call or contextual record.

Rule 1: Generic: Comprehensive Audit Trails
GUGUbot/gugubot/logic/system/execute.py[127-130]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Unauthorized `#execute` attempts are denied with `return False` but no audit trail is recorded. This violates the requirement to log critical actions with user ID, action description, timestamp, and outcome.
## Issue Context
`#execute` triggers server command execution and is security-sensitive. Denials (and optionally successful executions) should be logged in a structured way for later investigation.
## Fix Focus Areas
- GUGUbot/gugubot/logic/system/execute.py[127-130]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Default config inconsistent ☑ 🐞 Bug ⛯ Reliability

Description

The PR updates the repo-root default_config.yml, but the plugin actually generates/migrates config
from the bundled gugubot/config/defaults/default_config.yml, so users won’t receive the updated
guidance. Additionally, the repo-root default_config.yml has invalid YAML indentation for
ignore_execute_command_patterns, likely breaking parsing for anyone copying it.

Code

default_config.yml[R156-157]

  ignore_execute_command_patterns:   # 忽略的命令模式列表（使用正则表达式）
-    - ".*?give.*?" # 忽略 give 指令
+    - ".*?give.*?" # 忽略 give 指令，为空则不忽略任何指令

Evidence

Runtime config initialization/migration reads the bundled default config
(gugubot/config/defaults/default_config.yml), not the repo-root default_config.yml, so changing
only the root file is ineffective for actual users. Separately, the root file’s
ignore_execute_command_patterns list item is not indented under the key (dash aligned with the
key), which is invalid YAML and can fail parsing if used as a template.

GUGUbot/gugubot/init.py[36-51]
GUGUbot/gugubot/config/BotConfig.py[18-27]
default_config.yml[153-158]
GUGUbot/gugubot/config/defaults/default_config.yml[153-158]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR updates repo-root `default_config.yml`, but the plugin uses the bundled `gugubot/config/defaults/default_config.yml` for generating/migrating config. Also, the repo-root YAML has a mis-indented list under `ignore_execute_command_patterns`, making it invalid as a template.
### Issue Context
Config initialization/migration reads `gugubot/config/defaults/default_config.yml` via `open_bundled_file()`. Users copying the root `default_config.yml` may encounter YAML parse failures.
### Fix Focus Areas
- default_config.yml[153-158]
- GUGUbot/gugubot/config/defaults/default_config.yml[153-158]
- GUGUbot/gugubot/__init__.py[36-51]
- GUGUbot/gugubot/config/BotConfig.py[18-27]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Admin check ordering cost ☑ 🐞 Bug ➹ Performance

Description

The permission check awaits _is_admin() before consulting broadcast_info.is_admin, so it can
perform expensive permission resolution (including QQ group-member list calls) even when
broadcast_info.is_admin is already true; moving the check earlier also makes this happen before
any early returns (e.g., help/empty handling). This can increase latency and risk QQ API
rate-limit/instability under frequent command usage.

Code

GUGUbot/gugubot/logic/system/execute.py[R127-130]

+            # 权限检查：必须是管理员才能执行命令
+            if not self.is_command(broadcast_info) or \
+                    (not await self._is_admin(broadcast_info.sender_id) and not broadcast_info.is_admin):
+                return False

Evidence

In the new placement, the #execute branch performs the permission check immediately after the
prefix match and before parsing/handling help; the boolean expression evaluates `await
self._is_admin(...) as the left operand of and`, so it is awaited before checking
broadcast_info.is_admin. _is_admin() delegates to PlayerManager.is_admin(), which for the QQ
connector can call bot.get_group_member_list() (network I/O) for each configured admin group
without caching, making the check potentially slow and rate-limit-prone.

GUGUbot/gugubot/logic/system/execute.py[126-140]
GUGUbot/gugubot/utils/player_manager.py[221-263]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ExecuteSystem.process_broadcast_info()` awaits `_is_admin()` before checking `broadcast_info.is_admin`, causing unnecessary expensive permission lookups (potential QQ API calls) even when `broadcast_info.is_admin` already indicates admin.
### Issue Context
`PlayerManager.is_admin()` may call QQ `get_group_member_list()` per admin group, so avoiding unnecessary awaits reduces latency and rate-limit risk.
### Fix Focus Areas
- GUGUbot/gugubot/logic/system/execute.py[126-130]
- GUGUbot/gugubot/logic/system/execute.py[165-169]
- GUGUbot/gugubot/utils/player_manager.py[221-263]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more