Skip to content

Fix: style resets to default on bot reload#205

Open
NancalaStarry wants to merge 1 commit intoPFingan-Code:mainfrom
NancalaStarry:style-reload-fix
Open

Fix: style resets to default on bot reload#205
NancalaStarry wants to merge 1 commit intoPFingan-Code:mainfrom
NancalaStarry:style-reload-fix

Conversation

@NancalaStarry
Copy link

在输入!!MCDR plg reload的时候发现会将机器人的风格变成默认风格

@NancalaStarry NancalaStarry marked this pull request as ready for review March 1, 2026 05:03
@qodo-code-review
Copy link

Review Summary by Qodo

Fix bot style resetting to default on plugin reload

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Fix bot style resetting to default on plugin reload
• Correct config key path from nested GUGUBot to flat style structure
• Update style persistence to use proper config hierarchy
Diagram
flowchart LR
  A["Config Key Path"] -->|"Old: GUGUBot.style"| B["Style Reset Issue"]
  A -->|"New: style.current_style"| C["Style Persists"]
  D["_save_current_style"] -->|"Updated to use style config"| C
Loading

Grey Divider

File Changes

1. GUGUbot/gugubot/utils/style_manager.py 🐞 Bug fix +4/-5

Fix config key path for style persistence

• Changed config key path from ["GUGUBot", "style"] to ["style", "current_style"] in __init__
 method
• Updated _save_current_style method to use correct config hierarchy
• Removed nested GUGUBot config structure in favor of flat style config structure
• Ensures current style is properly persisted and restored on plugin reload

GUGUbot/gugubot/utils/style_manager.py


Grey Divider

Qodo Logo

@qodo-code-review
Copy link

qodo-code-review bot commented Mar 1, 2026

Code Review by Qodo

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

Grey Divider


Remediation recommended

1. No legacy style migration 🐞 Bug ✓ Correctness
Description
StyleManager now only reads/writes style.current_style; if an existing config stored the selected
style in a different legacy location, that value will be ignored and the bot may still appear to
reset style after reload/upgrade.
Code

GUGUbot/gugubot/utils/style_manager.py[83]

+            self.current_style = config.get_keys(["style", "current_style"], None)
Evidence
StyleManager reads current style exclusively from the root style.current_style key, matching the
default config schema. The provided migration utility only targets v1-style root keys and does not
address other potential legacy locations, and BotConfig’s merge routine does not perform schema
conversions beyond copying keys.

GUGUbot/gugubot/utils/style_manager.py[70-84]
GUGUbot/gugubot/utils/style_manager.py[357-362]
GUGUbot/gugubot/config/defaults/default_config.yml[115-120]
GUGUbot/gugubot/utils/config_migrator.py[139-144]
GUGUbot/gugubot/config/BotConfig.py[28-38]

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

### Issue description
After this PR, `StyleManager` only consumes `style.current_style`. Any existing config that still stores the last selected style in a legacy location will be ignored, causing style to appear to reset after reload/upgrade.

### Issue Context
- Default config uses `style.current_style`.
- There is no migration/fallback logic in `StyleManager.__init__()`.

### Fix Focus Areas
- GUGUbot/gugubot/utils/style_manager.py[70-84]
- GUGUbot/gugubot/utils/style_manager.py[357-362]
- (Optional) GUGUbot/gugubot/utils/config_migrator.py[139-144]

### Suggested approach
- In `__init__`, try `style.current_style` first.
- If missing/None, try legacy path(s) and, if found, assign to `self.current_style` and persist to `style.current_style` (and optionally delete the legacy key to avoid confusion).
- Ensure the migration is safe if legacy keys are absent.

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


2. Config type can crash 🐞 Bug ⛯ Reliability
Description
StyleManager’s new get_keys(["style","current_style"]) call can raise at startup if config["style"]
is not a dict (malformed or partially-migrated config), because get_keys assumes all intermediate
values support .get().
Code

GUGUbot/gugubot/utils/style_manager.py[83]

+            self.current_style = config.get_keys(["style", "current_style"], None)
Evidence
StyleManager now traverses into the style section via get_keys. BasicConfig.get_keys does not
validate intermediate types; if style exists but is a scalar, it will attempt to call .get() on
a non-dict and crash. Additionally, the v1->v2 migrator explicitly skips migration when GUGUBot
exists, so a config that is partially updated (contains GUGUBot but still has a legacy scalar
style) would not be auto-fixed and could hit this crash path.

GUGUbot/gugubot/utils/style_manager.py[81-84]
GUGUbot/gugubot/config/BasicConfig.py[51-55]
GUGUbot/gugubot/utils/config_migrator.py[26-29]

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

### Issue description
`BasicConfig.get_keys()` assumes all intermediate nodes are dicts. `StyleManager` now calls `get_keys(["style", "current_style"])`; if `config["style"]` exists but is not a dict (mis-edit or partial migration), plugin load can crash.

### Issue Context
`get_keys` uses `.get()` on intermediate values without type checks.

### Fix Focus Areas
- GUGUbot/gugubot/utils/style_manager.py[70-84]
- GUGUbot/gugubot/config/BasicConfig.py[51-55]

### Suggested approach
Option A (preferred, global safety):
- Update `BasicConfig.get_keys` to:
 - check `isinstance(result, dict)` each loop iteration
 - if not a dict, return `default`

Option B (local safety):
- In `StyleManager.__init__`, read `style_section = config.get('style')`; if not a dict, log a warning and treat as missing (use None/default).

Add a warning log so operators can fix their config.

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


Grey Divider

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

Grey Divider

Qodo Logo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant