|
| 1 | +# Known Issues & Accepted Risks |
| 2 | + |
| 3 | +> Last audited: 2025-02-18 |
| 4 | +> Context: Single-user personal Discord bot — not a production multi-user service |
| 5 | +
|
| 6 | +This document tracks issues identified during a comprehensive code audit that were **intentionally not fixed**. Each entry explains why the issue exists, why it doesn't matter for our use case, and under what conditions (if any) it would need revisiting. |
| 7 | + |
| 8 | +--- |
| 9 | + |
| 10 | +## Security — Accepted for Personal Use |
| 11 | + |
| 12 | +### Command Injection via `/git` (Audit #2) |
| 13 | + |
| 14 | +**What:** The `/git` command passes user input to `git` CLI without sanitization. A crafted input could inject shell commands. |
| 15 | + |
| 16 | +**Why we keep it:** This bot is operated by its owner in a private Discord server. The only person who can run `/git` is the same person who has SSH access to the machine. You can't escalate beyond what you already have. If the bot were ever exposed to untrusted users, this would need parameterized command execution. |
| 17 | + |
| 18 | +### Path Traversal in Git Worktree (Audit #3) |
| 19 | + |
| 20 | +**What:** The `/git worktree` subcommand accepts paths without restriction, so `../../etc/passwd` style paths are technically possible. |
| 21 | + |
| 22 | +**Why we keep it:** Same reasoning — the operator already has full filesystem access. The bot doesn't expand the attack surface beyond what the user already controls. |
| 23 | + |
| 24 | +### `/env-vars` Exposes Environment (Audit #4) |
| 25 | + |
| 26 | +**What:** The `/env-vars` system command displays environment variables, which could include tokens or secrets. |
| 27 | + |
| 28 | +**Why we keep it:** This is a debugging utility for the bot owner. The Discord channel is private. The bot token is already known to the operator. If you add RBAC restrictions to limit who can run system commands, this becomes a non-issue even in shared servers. |
| 29 | + |
| 30 | +### Full Environment Propagated to Child Processes (Audit #19) |
| 31 | + |
| 32 | +**What:** When spawning git supervisor sub-bots, the full environment (including `DISCORD_TOKEN`) is passed to child processes. |
| 33 | + |
| 34 | +**Why we keep it:** Child processes run on the same machine under the same user. They inherit the same trust boundary. The token is needed for the child bot to function. There's no privilege escalation. |
| 35 | + |
| 36 | +--- |
| 37 | + |
| 38 | +## Memory — Self-Resolving |
| 39 | + |
| 40 | +### `expandableContent` Map Grows Unboundedly (Audit #5) |
| 41 | + |
| 42 | +**What:** Every "Show Full" button creates an entry in a Map that's never evicted. Over time, memory usage grows. |
| 43 | + |
| 44 | +**Why it doesn't matter:** For a single user generating maybe 10-50 expandable messages per day, this is a few KB. The bot process restarts periodically (updates, system reboots, crashes), which clears the Map. You'd need to run the bot for months with heavy usage to notice any impact. |
| 45 | + |
| 46 | +**When to revisit:** If the bot runs continuously for weeks with very heavy use and you notice memory growth in `htop`/Task Manager. |
| 47 | + |
| 48 | +### Agent Sessions Never Cleaned (Audit #18) |
| 49 | + |
| 50 | +**What:** The `agentSessions` Map in `agent/index.ts` accumulates session objects that are never removed. |
| 51 | + |
| 52 | +**Why it doesn't matter:** Each session is a small object. A single user creates maybe 1-5 sessions per day. Bot restarts clear them. Same as above — self-resolving through natural process lifecycle. |
| 53 | + |
| 54 | +### Usage Records Lost on Crash (Audit #17) |
| 55 | + |
| 56 | +**What:** Usage data is batched (saved every 5 records). On crash, up to 4 records are lost. |
| 57 | + |
| 58 | +**Why it doesn't matter:** Usage tracking is informational, not billing. Losing a few records has zero functional impact. The data is nice-to-have, not critical. |
| 59 | + |
| 60 | +--- |
| 61 | + |
| 62 | +## Race Conditions — Single User Can't Trigger |
| 63 | + |
| 64 | +### Concurrent Query State Clobber (Audit #6) |
| 65 | + |
| 66 | +**What:** If two `/claude` commands run simultaneously, they share a single `claudeController`. Only the last one can be cancelled. |
| 67 | + |
| 68 | +**Why it doesn't matter:** As a single user, you won't send two `/claude` commands at the exact same time. Even if you did, both queries still complete — you just can't cancel the first one. The second command's abort check (fixed in commit `5366f00`) now properly aborts the first. |
| 69 | + |
| 70 | +### Shell Callback Registration Race (Audit #7) |
| 71 | + |
| 72 | +**What:** Two simultaneous `/shell` commands could cross-wire their output callbacks. |
| 73 | + |
| 74 | +**Why it doesn't matter:** Same as above — single user, sequential usage. You'd have to fire two shell commands within milliseconds of each other. |
| 75 | + |
| 76 | +--- |
| 77 | + |
| 78 | +## Non-Functional / Dead Code |
| 79 | + |
| 80 | +### Proxy Implementation Non-Functional (Audit #8) |
| 81 | + |
| 82 | +**What:** The proxy configuration code in `util/proxy.ts` exists but doesn't actually apply the proxy to outgoing requests. |
| 83 | + |
| 84 | +**Why we keep it:** If you're not behind a proxy, this code is never invoked. It's dead code that doesn't hurt anything. If proxy support is needed in the future, the scaffolding is there to build on. |
| 85 | + |
| 86 | +### `removeSignalHandlers` Is a No-Op (Audit #23) |
| 87 | + |
| 88 | +**What:** The function to remove signal handlers doesn't actually store handler references, so it can't remove them. |
| 89 | + |
| 90 | +**Why it doesn't matter:** Signal handlers only fire on SIGINT/SIGTERM (process shutdown). The "remove" function would only matter for hot-reload scenarios, which don't apply to this bot. On shutdown, all handlers fire and cleanup runs — the fact that you can't selectively remove them is irrelevant. |
| 91 | + |
| 92 | +### `isValidModel` Always Returns True (Audit #16) |
| 93 | + |
| 94 | +**What:** The model validation function accepts any string as a valid model name. |
| 95 | + |
| 96 | +**Why it doesn't matter:** If you type an invalid model name, the Anthropic API returns an error. You get the same outcome (failure + error message) whether validation happens client-side or server-side. The API is the authoritative validator. |
| 97 | + |
| 98 | +### `getEnvVarPattern` Identical on Both Platforms (Audit #26) |
| 99 | + |
| 100 | +**What:** The Windows and Unix branches of `getEnvVarPattern` return the same regex. |
| 101 | + |
| 102 | +**Why it doesn't matter:** It works correctly on both platforms. The code is just redundant, not buggy. |
| 103 | + |
| 104 | +--- |
| 105 | + |
| 106 | +## Fragile but Functional |
| 107 | + |
| 108 | +### Duplicate Signal Handlers (Audit #15) |
| 109 | + |
| 110 | +**What:** Multiple modules register their own SIGINT/SIGTERM handlers. On shutdown, all of them fire, which means cleanup code runs multiple times. |
| 111 | + |
| 112 | +**Why it doesn't matter:** The cleanup operations are idempotent (aborting an already-aborted controller, destroying an already-destroyed client). Multiple runs don't cause errors. At worst, you see extra log lines during shutdown. |
| 113 | + |
| 114 | +### RBAC Cache Never Invalidated (Audit #14) |
| 115 | + |
| 116 | +**What:** Permission check results are cached and never expire. If you change role assignments, the cache still has the old results. |
| 117 | + |
| 118 | +**Why it doesn't matter:** As the only user, your permissions don't change. Even if they did, a bot restart clears the cache. For a multi-user setup, you'd want TTL-based invalidation. |
| 119 | + |
| 120 | +### Model List Parser Is Regex-Based (Audit #20) |
| 121 | + |
| 122 | +**What:** The `/claude-models` command parses `claude` CLI output with regex. If Anthropic changes the output format, parsing breaks. |
| 123 | + |
| 124 | +**Why we accept it:** There's no stable API for this. Regex parsing is the only option. When it breaks, it fails gracefully (shows empty list or error), and the fix is a regex update. |
| 125 | + |
| 126 | +### Hardcoded Rate Limit Tiers (Audit #24) |
| 127 | + |
| 128 | +**What:** Rate limit display values are hardcoded rather than fetched from the API. |
| 129 | + |
| 130 | +**Why it doesn't matter:** These are informational displays. The actual rate limiting is enforced by Anthropic's API, not by this bot. Displaying slightly outdated tier info has no functional impact. |
| 131 | + |
| 132 | +### Manual Settings Sync (Audit #27) |
| 133 | + |
| 134 | +**What:** Multiple settings systems (advanced, unified, legacy) must be manually kept in sync. Changing a setting in one place might not propagate. |
| 135 | + |
| 136 | +**Why we accept it:** The unified settings system is the primary one. Legacy systems exist for backward compatibility. In practice, the user interacts with one settings interface. If something feels out of sync, re-setting the value fixes it. |
| 137 | + |
| 138 | +### `fetchClaudeInfo` Subprocess Leak (Audit #9) |
| 139 | + |
| 140 | +**What:** The `claude` CLI subprocess spawned by `query-manager.ts` may not be killed on timeout or abort. |
| 141 | + |
| 142 | +**Why it doesn't matter:** A stray `claude` process eventually exits on its own (API timeout, idle timeout). For a single user, at most one stray process exists at a time. The OS handles cleanup on bot restart. |
| 143 | + |
| 144 | +--- |
| 145 | + |
| 146 | +## Edge Cases — Rare in Practice |
| 147 | + |
| 148 | +### JSON Streaming Empty Response (Audit #21) |
| 149 | + |
| 150 | +**What:** Certain response formats in the JSON streaming path could return an empty string. |
| 151 | + |
| 152 | +**Why it's rare:** This path is only triggered with specific Claude API response shapes. In normal conversational use, it doesn't occur. If it does, you just re-run the command. |
| 153 | + |
| 154 | +### Unawaited Screenshot Cleanup (Audit #22) |
| 155 | + |
| 156 | +**What:** Temporary screenshot files might not be deleted if the async cleanup doesn't complete. |
| 157 | + |
| 158 | +**Why it doesn't matter:** A few stray temp files in `/tmp` or `%TEMP%`. The OS cleans these periodically. Zero functional impact. |
| 159 | + |
| 160 | +### `detectPlatform` Uncached (Audit #25) |
| 161 | + |
| 162 | +**What:** Platform detection runs every time it's called instead of caching the result. |
| 163 | + |
| 164 | +**Why it doesn't matter:** It's a few string comparisons and property reads. Microseconds per call. Not a performance bottleneck by any measure. |
| 165 | + |
| 166 | +--- |
| 167 | + |
| 168 | +## Summary |
| 169 | + |
| 170 | +| Category | Count | Action | |
| 171 | +|----------|-------|--------| |
| 172 | +| Security (accepted for personal use) | 4 | No fix needed — same trust boundary | |
| 173 | +| Memory (self-resolving) | 3 | Bot restarts clear state | |
| 174 | +| Race conditions (single-user) | 2 | Can't trigger with sequential usage | |
| 175 | +| Dead/non-functional code | 4 | No impact, leave as-is | |
| 176 | +| Fragile but functional | 6 | Works in practice, accept the tradeoff | |
| 177 | +| Rare edge cases | 3 | Not worth the complexity to fix | |
| 178 | +| **Total accepted** | **22** | | |
| 179 | + |
| 180 | +**Issues that were fixed:** #1 (stale closure), #10 (missing abort checks), #11 (pagination title), #12 (unicode splitting), #13 (continue button session ID) — see git log for details. |
0 commit comments