refactor(plugins): comprehensive code review - ~35 fixes across 14 plugins
Phase 1 - Plugin code review (14/14 plugins): - Security: 3x token leak in print→logger.debug, Bearer prefix handling - Bug: bare except→specific exceptions, HorseState type safety, sync→async - Critical: response_model undefined, route dead code, sync blocking event loop - Quality: 11x print()→logger, variable name shadowing, consistent logging Phase 2 - Deep analysis: - Fix: payout int truncation→max(1, round(amount*odds)) - Fix: room_store get_lock race condition→dict.setdefault() - Verify: data_manager f-string SQL is safe (uses ? placeholders) Infrastructure: review reports generated for all plugins.
This commit is contained in:
52
danding_bot/plugins/group_horse_racing/REVIEW_REPORT.md
Normal file
52
danding_bot/plugins/group_horse_racing/REVIEW_REPORT.md
Normal file
@@ -0,0 +1,52 @@
|
||||
# Code Review Report: group_horse_racing
|
||||
**Date**: 2026-05-09
|
||||
**Scope**: 15 Python files, ~200KB
|
||||
**Files Modified**: 4
|
||||
|
||||
## Summary
|
||||
Horse racing plugin with room management, betting system, race simulation, and settlement.
|
||||
Overall architecture is clean (command pattern + engine + store separation). Found 1 critical singleton bug and 1 enum bug.
|
||||
|
||||
## Issues Found & Fixed
|
||||
|
||||
### FIX 1 — CRITICAL: Dual RoomStore Instances (shared.py)
|
||||
- **Problem**: `shared.py` created its own `RoomStore(Config())` at module level (L18), separate from the singleton in `room_store.py` (L253) managed by `__init__.py` lifecycle hooks. This meant the `startup`/`shutdown` hooks (init DB, cleanup old rooms) operated on a DIFFERENT instance than the commands module.
|
||||
- **Impact**: Race data persistence could silently fail — rooms might not save to DB, old rooms not cleaned up.
|
||||
- **Fix**: Changed `from ..room_store import RoomStore` + `room_store = RoomStore(config)` → `from ..room_store import room_store` (import the singleton).
|
||||
- **Risk**: Low — straightforward import change.
|
||||
|
||||
### FIX 2 — BUG: Invalid HorseState.WAITING (race.py L146)
|
||||
- **Problem**: After stopping a race, horses were set to `HorseState.WAITING` which doesn't exist in the enum (only READY/RACING/FINISHED).
|
||||
- **Impact**: Would raise `AttributeError` at runtime if stop-race command was used.
|
||||
- **Fix**: Changed to `HorseState.READY`.
|
||||
- **Risk**: None — enum value now exists.
|
||||
|
||||
### FIX 3 — Silent Exceptions → Debug Logging (message_service.py)
|
||||
- **Problem**: Two `except Exception: pass` blocks in `recall_previous_of_type` (L66) and `_schedule_recall` (L81).
|
||||
- **Context**: Message deletion failures (network errors, already deleted).
|
||||
- **Fix**: Added `logger.debug(..., exc_info=True)` for observability.
|
||||
- **Risk**: None — logging only.
|
||||
|
||||
### FIX 4 — Silent Exceptions → Debug Logging (test_commands.py)
|
||||
- **Problem**: Two `except Exception: pass` blocks in test cleanup code (L256, L237).
|
||||
- **Fix**: Added `logger.debug(...)` for test debugging.
|
||||
|
||||
## Issues Reviewed & Accepted (No Fix Needed)
|
||||
- **config.py:75** — Silent `except ValueError: pass` with fallback to `set()`. Already has warning at L70. Radius-0 operation.
|
||||
- **race.py:77,127** — Admin check silent excepts. Default to non-admin on API failure. Radius-0 operation.
|
||||
- **shared.py:31** — Name lookup fallback to user_id string. Radius-0 operation.
|
||||
- **test_commands.py L266** — `RoomStore()` in `_InMemoryRoomStore` mock. Test-only, acceptable.
|
||||
- **image_render.py** — PIL synchronous rendering. Pre-existing in qqpush plugin (fixed there). Not actionable here as it's the same shared code.
|
||||
|
||||
## Architecture Notes
|
||||
- Good separation: RoomStore (persistence) → RaceEngine (logic) → MessageService (messaging) → Commands (handlers)
|
||||
- Singleton pattern for RoomStore with lifecycle management via nonebot hooks
|
||||
- Race simulation runs as asyncio task with tick-based updates
|
||||
- Betting system with odds calculation is well-structured
|
||||
- Test file (413 lines) provides good simulation coverage
|
||||
|
||||
## Verification
|
||||
- ✅ 15/15 files syntax valid
|
||||
- ✅ No `HorseState.WAITING` references remain
|
||||
- ✅ `shared.py` imports singleton (no `RoomStore(config)` call)
|
||||
- ✅ Debug logging present in message_service.py and test_commands.py
|
||||
Reference in New Issue
Block a user