|
| 1 | +# Implementation Plan — Codebase Audit 2026-04-23 12:27 |
| 2 | + |
| 3 | +## Overview |
| 4 | + |
| 5 | +16 findings total (0 Critical, 2 High, 6 Medium, 8 Low). Estimated effort: ~9 S, ~5 M, ~2 L tasks. All prior audit findings (17/17) are resolved — this plan covers new findings only. |
| 6 | + |
| 7 | +## Phase 1: Security & Race Conditions (Do First) |
| 8 | + |
| 9 | +| # | Finding | Files | Effort | Description | |
| 10 | +|---|---------|-------|--------|-------------| |
| 11 | +| 1 | H1 — Unsanitized tool results fed to LLM | `src/services/nl_processor.py:285`, `src/services/nl_tools.py:462` | M | In `nl_processor.py:285`, wrap tool result: `from src.utils.sanitize import sanitize_for_prompt; result = sanitize_for_prompt(result, max_length=8000)` before appending to `tool_results`. Also sanitize error text in `_tool_get_recent_errors()` at nl_tools.py:462. | |
| 12 | +| 2 | H2 — MemoryMonitor kill/decline race | `src/monitors/memory_monitor.py:162-214` | M | Add `self._kill_lock = asyncio.Lock()` to `__init__`. Wrap `_execute_kill_countdown()` lines 162-207 in `async with self._kill_lock`. In `cancel_pending_kill()`, also acquire the lock. Re-check `_kill_cancel_event.is_set()` immediately before `_stop_container()` at line 196. | |
| 13 | +| 3 | M2 — Uncapped mute duration in callbacks | `src/bot/alert_callbacks.py:267-270` | S | After `minutes = int(parts[1])`, add `minutes = min(max(minutes, 1), 10080)` to cap at 7 days. | |
| 14 | +| 4 | M3 — World-writable ignore files | `src/alerts/ignore_manager.py:359` | S | Change `os.fchmod(fd, 0o666)` to `os.fchmod(fd, 0o644)`. | |
| 15 | + |
| 16 | +## Phase 2: Data Integrity |
| 17 | + |
| 18 | +| # | Finding | Files | Effort | Description | |
| 19 | +|---|---------|-------|--------|-------------| |
| 20 | +| 5 | M1 — ChatIdStore non-atomic writes | `src/alerts/manager.py:45-54` | M | Replace bare `json.dump()` with tempfile + `os.replace()` pattern matching `base_mute_manager.py:149-159`. Add `import os, tempfile`. Use `tempfile.mkstemp(dir=..., prefix=".tmp_chatids_", suffix=".json")`, write, `os.fchmod(fd, 0o644)`, `os.replace()`. | |
| 21 | +| 6 | M4 — BOT_VERSION stale | `src/bot/health_command.py:23` | S | Replace `BOT_VERSION = "0.9.2"` with dynamic version: `from importlib.metadata import version; BOT_VERSION = version("unraid-monitor-bot")` wrapped in try/except `PackageNotFoundError` with fallback to `"unknown"`. | |
| 22 | + |
| 23 | +## Phase 3: Memory & Scalability |
| 24 | + |
| 25 | +| # | Finding | Files | Effort | Description | |
| 26 | +|---|---------|-------|--------|-------------| |
| 27 | +| 7 | M5 — CrashTracker unbounded dict keys | `src/monitors/docker_events.py:35-103` | M | Add a `_cleanup_stale()` method that removes entries from `_crashes` where the list is empty, and entries from `_last_escalation`/`_last_recovery_alert` older than 24 hours. Call it from `record_crash()` every 100 calls (counter pattern, like RateLimiter). | |
| 28 | +| 8 | M6 — Pattern cache unbounded | `src/monitors/log_watcher.py:26-27` | S | Replace mutable default dict with a module-level `_PATTERN_CACHE: dict = {}` and add `_PATTERN_CACHE_MAX = 64`. In the function, check `if len(_PATTERN_CACHE) > _PATTERN_CACHE_MAX: _PATTERN_CACHE.clear()`. | |
| 29 | + |
| 30 | +## Phase 4: Code Quality & Cleanup |
| 31 | + |
| 32 | +| # | Finding | Files | Effort | Description | |
| 33 | +|---|---------|-------|--------|-------------| |
| 34 | +| 9 | L1 — Redundant asyncio imports | `src/monitors/resource_monitor.py:178, 205` | S | Delete `import asyncio` at lines 178 and 205. Module-level import at line 1 is sufficient. | |
| 35 | +| 10 | L2 — Redundant time import | `src/monitors/log_watcher.py:217` | S | Delete `import time` at line 217. Module-level import at line 4 is sufficient. | |
| 36 | +| 11 | L3 — Health command encapsulation | `src/bot/health_command.py`, `src/monitors/*.py` | L | Add public status properties to each monitor: `DockerEventMonitor.is_running`, `LogWatcher.is_running`, `LogWatcher.total_drops`, etc. Update health_command.py to use these instead of private attributes. | |
| 37 | +| 12 | L6 — Missing return in telegram_retry | `src/utils/telegram_retry.py:92` | S | Add explicit `return None` after the for loop at line 92 to satisfy mypy. | |
| 38 | +| 13 | L8 — parse_duration unbounded | `src/alerts/mute_manager.py:33` | S | After `if value <= 0: return None`, add `if value > 10080: return None` to cap at 7 days (10080 minutes). | |
| 39 | +| 14 | L4 — NLProcessor._user_locks TTL | `src/services/nl_processor.py:203-206` | S | Add timestamp tracking to cleanup: only delete locks for users inactive >30 minutes, regardless of lock state. | |
| 40 | + |
| 41 | +## Phase 5: Type Safety (Incremental) |
| 42 | + |
| 43 | +| # | Finding | Files | Effort | Description | |
| 44 | +|---|---------|-------|--------|-------------| |
| 45 | +| 15 | L7 — mypy errors (191) | Various (30 files) | L | Address incrementally. Priority order: (a) Add `types-PyYAML` to dev dependencies to fix `import-untyped`. (b) Add return type annotations to all factory functions in `src/bot/`. (c) Add generic type parameters to `dict`, `list`, `Task` uses. (d) Fix `InaccessibleMessage` union handling by adding runtime isinstance checks in callback handlers. | |
| 46 | +| 16 | L5 — OpenAI prompt caching | `src/services/llm/openai_provider.py` | M | OpenAI auto-caches repeated prompts server-side (no code change needed for basic caching). For explicit caching, would need to use their `cached_tokens` API — defer until cost becomes an issue. | |
| 47 | + |
| 48 | +## Dependencies & Ordering Notes |
| 49 | + |
| 50 | +- **H1 must come first** — it's a security fix that prevents prompt injection via crafted container logs. |
| 51 | +- **H2 (kill/decline race)** is independent and can be done in parallel with H1. |
| 52 | +- **M1 (atomic writes)** and **M4 (BOT_VERSION)** are independent quick wins. |
| 53 | +- **Items 9-14** (Phase 4) are all independent and can be done in parallel or batched into a single commit. |
| 54 | +- **L7 (mypy errors)** is best done incrementally over multiple sessions — don't try to fix all 191 at once. |
| 55 | + |
| 56 | +## Quick Wins (S effort + High/Medium severity) |
| 57 | + |
| 58 | +These should be done first within each phase: |
| 59 | +1. M2 — Cap mute duration (S) — one line change |
| 60 | +2. M3 — Fix file permissions (S) — one line change |
| 61 | +3. M4 — Fix BOT_VERSION (S) — two line change |
| 62 | +4. M6 — Bound pattern cache (S) — five line change |
0 commit comments