Files

Mr.Xia c01338f496 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.

2026-05-09 23:22:28 +08:00

3.3 KiB

Raw Permalink Blame History

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

3.3 KiB Raw Permalink Blame History