COMPLIANCE.md 21 KB
Compliance Review -- discordo-plus
Reviewed: 2026-03-20 | Reviewer: Claude (automated)
Scope: all source files | Standard: General (Code Quality, Performance, Go Best Practices)
Reference: research/TECHFILE.md for dependency and version context.
CRITICAL
1. ✅ FIXED — Cache.Get Panics on Missing Key
internal/cache/cache.go:27 -- The Get method performs an unchecked type assertion i.(uint) on the result of sync.Map.Load. If the key does not exist, i is nil and the assertion panics. While Exists is called before Get in the current codebase (message_input.go:499-500), there is no guarantee callers will always check first, and the race window between Exists and Get on a sync.Map means concurrent Invalidate could delete the key between the two calls, causing a runtime panic.
Fix: Change Get to return (uint, bool) or use a comma-ok assertion: if i, ok := c.items.Load(query); ok { return i.(uint) }; return 0. Update callers to handle the zero/missing case.
Implementation Risk: Low. Only message_input.go calls Get, and the change is a straightforward API update. The fix also closes a potential TOCTOU race.
2. ✅ FIXED — HTTP Response Body Leak in Brotli Transport
internal/http/transport.go:28-32 -- When the response has Content-Encoding: br, the original resp.Body is wrapped with brotli.NewReader via io.NopCloser. This means calling Close() on the new body is a no-op and the original body's connection is never returned to the pool. For long-running applications like a Discord client, this can exhaust available connections over time.
Fix: Replace io.NopCloser(brotli.NewReader(resp.Body)) with a custom ReadCloser that wraps the brotli reader but delegates Close() to the original resp.Body. Example: type brotliReadCloser struct { io.Reader; closer io.Closer } with func (b *brotliReadCloser) Close() error { return b.closer.Close() }.
Implementation Risk: Low. The fix is a small wrapper struct. Verify with a test that the underlying body is closed when the brotli-wrapped response body is closed.
3. ✅ FIXED — Notification Avatar Download Uses Bare http.Get Without Timeout
internal/notifications/notifications.go:93 -- http.Get(url) uses the default HTTP client which has no timeout. A slow or unresponsive Discord CDN server will block the goroutine indefinitely. Since notifications are triggered from gateway events, this could accumulate blocked goroutines during network issues.
Fix: Use http.Client{Timeout: 10 * time.Second} or create a request with context.WithTimeout and pass it to http.DefaultClient.Do(req). Apply the same fix to downloadToCache in messages_list.go:1191 which also uses bare http.Get.
Implementation Risk: Low. Adding a timeout may cause some avatar downloads to fail under poor network conditions, but this is preferable to indefinite blocking. Existing error handling already covers the failure path.
4. ✅ FIXED — Log File Opened With os.ModePerm (0777)
internal/logger/logger.go:24 -- The log file is opened with os.ModePerm (0777), meaning it is world-readable and world-writable. Log files may contain sensitive debugging information (user IDs, channel names, error messages). Similarly, internal/consts/consts.go:25 creates the cache directory with os.ModePerm.
Fix: Use 0600 for the log file (owner read/write only) and 0700 for the cache directory (owner only). Apply the same change to guildstate.go:45 which writes state with 0644 (already better but could be 0600 for consistency).
Implementation Risk: Low. Users running in shared environments benefit from restrictive permissions. Existing users may need to manually fix permissions on existing files if they switch.
CODE QUALITY
5. ✅ FIXED — messages_list.go Is Over 1600 Lines -- God File
internal/ui/chat/messages_list.go -- Extracted into focused files: (a) embed_renderer.go for embed types, styles, line wrapping, URL helpers, and markdown unescape; (b) attachment_handler.go for download, view, save, and overlay methods; (c) url_extractor.go for URL extraction from message content and embeds. The main file now focuses on message rendering and keybinds.
6. ✅ FIXED — Duplicated ShortHelp/FullHelp Logic in guilds_tree.go
internal/ui/chat/guilds_tree.go:67-132 -- The ShortHelp() and FullHelp() methods contain nearly identical logic for determining the selectDesc string based on node state (lines 69-86 duplicated at 100-117). This 18-line block is copy-pasted.
Fix: Extract a helper method func (gt *guildsTree) selectActionDesc() string that returns "collapse", "expand", or the default description based on node state. Call it from both ShortHelp and FullHelp.
Implementation Risk: Low. Pure refactoring of display logic with no behavioral change.
7. ✅ FIXED — Duplicated attachmentsPicker Layer Setup
internal/ui/chat/messages_list.go:1178-1187 and internal/ui/chat/messages_list.go:1318-1327 -- The code to show the attachments picker overlay is identical in showAttachmentsList and saveImage: create centered layer, add with overlay options, send to front, set focus. This 10-line pattern is duplicated verbatim.
Fix: Extract a method func (ml *messagesList) showAttachmentsOverlay() that handles the layer setup and focus. Call it from both showAttachmentsList and saveImage after setting items.
Implementation Risk: Low. Simple extraction of identical code.
8. ✅ FIXED — Inconsistent Error Handling: Some Paths Silently Ignore Errors
internal/ui/chat/messages_list.go:1409 -- me, _ := ml.chatView.state.Cabinet.Me() discards the error. If the state is not ready, me is nil and me.ID will panic on the next line. This pattern appears in multiple places: state.go:195, messages_list.go:1446, messages_list.go:1539, messages_list.go:1563.
Fix: Handle the error case: me, err := ml.chatView.state.Cabinet.Me(); if err != nil { slog.Error(...); return }. Apply to all instances where Me() is called with a discarded error.
Implementation Risk: Low. Adding error checks prevents nil-pointer panics. The Me() call should always succeed after login, but defensive coding prevents crashes during edge-case race conditions (e.g., state reconnection).
9. ✅ FIXED — Magic Number: embedLineStyles Uses Hardcoded Array Size [8]
internal/ui/chat/messages_list.go:670 -- [8]tcell.Style{} is a magic number that must manually stay in sync with the number of embedLineKind constants (lines 660-668). Adding a new embed line kind without updating the array size causes a silent bug (zero-value style).
Fix: Define a sentinel const embedLineKindCount = 8 after the last iota constant, or use a slice. Example: add embedLineKindCount as the last iota entry and use [embedLineKindCount]tcell.Style{}.
Implementation Risk: Low. Compile-time enforcement prevents future mismatches.
10. ✅ FIXED — guildState Uses Mutex Where RWMutex Would Be Appropriate
internal/ui/chat/guildstate.go:14-17 -- guildState uses sync.Mutex for all access, but isExpanded (line 61) is a read-only operation that could use RLock. Guild state is read frequently (every node creation during onReady) and written infrequently (user expand/collapse action).
Fix: Change mu sync.Mutex to mu sync.RWMutex and use gs.mu.RLock()/gs.mu.RUnlock() in isExpanded.
Implementation Risk: Low. Standard Go concurrency pattern upgrade.
11. ✅ FIXED — Inconsistent Log Level Usage
Multiple files -- Fixed: keyring retrieval failure (root/events.go) and avatar cache failure (notifications.go) now log at slog.Warn instead of slog.Info. Presence lookup (message_input.go) remains at slog.Info as it is a normal condition. Added nil guard for Me() in state.go:onTypingStart to prevent potential panic.
12. ✅ FIXED — Invalid Log Level Silently Defaults to Zero Value
cmd/root.go:29-39 -- The switch on logLevel has no default case. An invalid value like --log-level=trace silently results in slog.Level(0) which is slog.LevelInfo. The user gets no feedback that their log level was invalid.
Fix: Add a default case that either returns an error (return fmt.Errorf("unknown log level: %q", logLevel)) or logs a warning and falls back to info.
Implementation Risk: Low. Adds user feedback for misconfiguration.
DOCUMENTATION GAPS
13. ✅ FIXED — No File-Level Comments on Most Source Files
Added package-level doc comments to internal/ui/chat/ (package comment in model.go) and internal/markdown/ (package comment in renderer.go). File-level doc comment added to events.go explaining the event/command architecture.
14. ✅ FIXED — No Documentation on the Event/Command Architecture
Added doc comment to internal/ui/chat/events.go explaining the command/event pattern: commands run off the main goroutine and return events dispatched to HandleEvent on the UI thread. Package-level doc in model.go describes the overall architecture.
15. ✅ FIXED — No Documentation on the Rendering Pipeline
Added package-level doc comment to internal/markdown/renderer.go explaining the AST→styled lines pipeline: goldmark AST walking, tcell style application for inline/block elements, and Discord-specific nodes (mentions, emoji, OSC 8 URLs).
16. ✅ FIXED — Config Validation Rules Undocumented
Added validation comments to AutocompleteLimit and MessagesLimit fields in config.go. Added DISCORDO_TOKEN security warning to config.toml. Improved image_viewer_args documentation in config.toml.
PERFORMANCE
17. Unbounded itemByID Cache in messagesList
internal/ui/chat/messages_list.go:58 -- itemByID map[discord.MessageID]*tview.TextView caches rendered message TextViews and is only cleared on channel switch (reset()) or when messages are modified. For channels with heavy traffic, this map grows without bound as new messages arrive. Each tview.TextView holds rendered line data, styles, and a text buffer. Over a long session in a busy channel, this can accumulate thousands of entries.
Fix: Add a size cap. When len(itemByID) > maxCacheSize (e.g., 500), evict entries for message IDs no longer in the current messages slice. Alternatively, clear the cache when it exceeds a threshold during addMessage.
Implementation Risk: Low-medium. Cache eviction during message additions may cause brief re-rendering of older messages if the user scrolls up. The visual impact is negligible since re-rendering is the fallback path.
18. ✅ FIXED — collapseParentNode Uses O(n) Tree Walk
internal/ui/chat/guilds_tree.go:380-395 -- collapseParentNode walks the entire tree from root to find the parent of the given node, even though GetPath(node) (used in canCollapseParent) returns the full path including the parent. The O(n) walk runs on every - keypress.
Fix: Use gt.GetPath(node) to get the parent directly: path := gt.GetPath(node); if len(path) >= 3 { parent := path[len(path)-2]; ... }. This is O(depth) instead of O(total nodes).
Implementation Risk: Low. GetPath is already used in canCollapseParent and expandPathToNode, so it is a known-reliable method. Verify the path includes the root node as the first element.
19. Repeated Full-Tree GetPath Calls in canCollapseParent
internal/ui/chat/guilds_tree.go:134-145 -- canCollapseParent is called from both ShortHelp() and FullHelp(), which run on every draw cycle to update the help bar. Each call invokes gt.GetPath(node) which walks the tree. Combined with ShortHelp and FullHelp duplicating the node-state checks, the help bar triggers 2+ tree walks per render frame.
Fix: Cache the path result for the current node, or compute canCollapseParent once and store it. Since the current node only changes on explicit user navigation, the cached value remains valid between navigation events.
Implementation Risk: Low. Cache invalidation happens naturally when SetCurrentNode is called.
20. extractURLs Parses Message Content Twice
internal/ui/chat/messages_list.go:1094-1115 and internal/ui/chat/messages_list.go:511-519 -- When rendering a message with embeds, drawEmbeds calls extractURLs(message.Content) to build a dedup set, and then drawContent also parses the same content through discordmd.ParseWithMessage for markdown rendering. The goldmark parser is invoked twice on the same content. For messages with complex markdown, this is measurable.
Fix: Parse content once and pass both the rendered lines and the extracted URLs to the embed renderer. This requires restructuring drawDefaultMessage to call a combined parse+render function that returns both outputs.
Implementation Risk: Medium. The rendering pipeline currently separates content rendering from embed rendering. Merging them requires careful handling of the forceMarkdown flag used in embed descriptions.
INFRASTRUCTURE
21. ✅ FIXED — CI Uses go-version: stable -- Not Pinned
.github/workflows/ci.yml:25 -- As documented in research/TECHFILE.md (finding #5), the CI uses go-version: stable which auto-advances when Go releases a new version. This can introduce unexpected breakage from new vet rules or behavior changes.
Fix: Pin to go-version: "1.26" or "1.26.x". Update intentionally when upgrading.
Implementation Risk: None from pinning. The risk is the current unpinned behavior.
22. No Linter in CI
.github/workflows/ci.yml -- The CI pipeline only runs go test and go build. There is no linter (golangci-lint, staticcheck, or go vet beyond what go build implicitly checks). Issues like unused variables, shadowed errors, and inefficient patterns are not caught in CI.
Fix: Add a lint step using golangci-lint with a .golangci.yml config. Start with default linters (vet, errcheck, staticcheck, unused) and gradually enable more.
Implementation Risk: Low for adding the step. Initial lint run may produce many findings that need to be triaged. Use --new-from-rev=HEAD~1 for incremental enforcement during transition.
23. ✅ FIXED — CI Artifact Name Does Not Match Binary Name
.github/workflows/ci.yml:41-43 -- CI uploads artifact named discordo_$OS_$ARCH but the binary is discordo (or discordo.exe). The project README and CLAUDE.md refer to the binary as discordo-plus. There is a naming inconsistency between the upstream module path (github.com/ayn2op/discordo), the CI artifact name, and the intended installation name.
Fix: Update the build step to go build -trimpath -ldflags=-s -o discordo-plus . (or discordo-plus.exe on Windows) to match the documented binary name. Update the artifact upload to reference the correct file.
Implementation Risk: Low. Downstream users of CI artifacts will see the renamed binary. Scoop/AUR packages may need path updates.
TEST COVERAGE
24. Only 2 Packages Have Tests -- Minimal Coverage
internal/config/config_test.go, internal/keyring/keyring_test.go -- Out of 12 internal packages, only 2 have any tests. Zero test coverage for: markdown rendering, clipboard operations, HTTP transport, notifications, cache, guild state persistence, UI components, embed processing, URL extraction, and the entire event handling pipeline.
Critical untested code paths:
cache.go:Get-- can panic (finding #1)transport.go-- body leak (finding #2)guildstate.go-- JSON marshal/unmarshal roundtripmessages_list.go--extractURLs,messageURLs,wrapStyledLine,unescapeMarkdownEscapes,linkDisplayText,sameLocalDateare all pure functions ideal for unit testingconfig/theme.go-- TOML unmarshaling for all wrapper typesutil.go--MergeStyle,SortGuildChannels,ChannelToString
Fix: Prioritize tests for: (a) pure functions in messages_list.go (URL extraction, wrapping, escaping), (b) cache.go roundtrip and race conditions, (c) guildstate.go persistence roundtrip, (d) theme.go unmarshaling edge cases. These require no mocking and have clear input/output contracts.
Implementation Risk: None. Tests are additive.
POSITIVES
- Go 1.26 module minimum is the latest stable release; dependency versions are current (see TECHFILE.md).
- Error wrapping with
%wis used consistently in config loading, HTTP transport, and attachment handling. - The event/command architecture cleanly separates I/O from UI updates; all gateway operations run in commands (closures), keeping the UI thread responsive.
- Config defaults are embedded via
//go:embed, eliminating runtime file-not-found failures. - Token storage uses the OS keyring (zalando/go-keyring) rather than plaintext files.
- Guild state persistence uses proper mutex synchronization around both reads and writes.
- The typing indicator implementation correctly uses
time.AfterFuncwith cleanup, preventing timer leaks. selectedChannelMuRWMutex correctly protects the selected channel across goroutines.- The markdown renderer's style stack pattern is clean and handles nested formatting correctly.
- The embed deduplication logic (
embedLineDedupKey) prevents duplicate content across embed fields. - Platform-specific code uses Go's filename-based build tag convention cleanly (clipboard, editor, suspend).
- The
ConfigurePickerhelper inutil.goavoids duplicating picker setup across three picker types. wrapStyledLinecorrectly uses grapheme cluster width viaunisegfor accurate wrapping with wide glyphs and emoji.
Summary
| Severity | Total |
|---|---|
| Critical | 4 |
| Code Quality | 8 |
| Documentation Gaps | 4 |
| Performance | 4 |
| Infrastructure | 3 |
| Test Coverage | 1 |
| Compliance | N/A |
Files Reviewed
| # | File | Type |
|---|---|---|
| 1 | main.go |
Entry point |
| 2 | cmd/root.go |
CLI / app init |
| 3 | internal/config/config.go |
Config struct + loader |
| 4 | internal/config/config.toml |
Embedded default config |
| 5 | internal/config/config_test.go |
Config tests |
| 6 | internal/config/keybinds.go |
Keybind definitions |
| 7 | internal/config/theme.go |
Theme TOML unmarshaling |
| 8 | internal/config/editor.go |
Editor command (merged from editor_unix.go + editor_default.go) |
| 10 | internal/consts/consts.go |
App name + cache dir |
| 11 | internal/logger/logger.go |
slog file logger |
| 12 | internal/keyring/keyring.go |
Token keyring wrapper |
| 13 | internal/keyring/keyring_test.go |
Keyring tests |
| 14 | internal/cache/cache.go |
Autocomplete cache |
| 15 | internal/http/transport.go |
HTTP transport (gzip + brotli) |
| 16 | internal/http/client.go |
API client constructor |
| 17 | internal/http/headers.go |
Request headers |
| 18 | internal/http/props.go |
Discord identify properties |
| 19 | internal/notifications/notifications.go |
Notification dispatch |
| 20 | internal/notifications/desktop_toast.go |
Non-darwin notification |
| 21 | internal/notifications/desktop_toast_darwin.go |
macOS notification |
| 22 | internal/clipboard/clipboard.go |
Clipboard format types |
| 23 | internal/clipboard/clipboard_default.go |
Default clipboard impl |
| 24 | internal/clipboard/clipboard_wayland.go |
Wayland clipboard impl |
| 25 | internal/markdown/renderer.go |
Discord markdown renderer |
| 26 | internal/ui/util.go |
Shared UI helpers |
| 27 | internal/ui/root/model.go |
Root model + help overlay |
| 28 | internal/ui/root/events.go |
Root events (token, clipboard) |
| 29 | internal/ui/root/keybinds.go |
Root keymap |
| 30 | internal/ui/root/suspend_unix.go |
Unix SIGTSTP suspend |
| 31 | internal/ui/root/suspend_default.go |
No-op suspend |
| 32 | internal/ui/chat/model.go |
Chat model + layout |
| 33 | internal/ui/chat/state.go |
Gateway state commands |
| 34 | internal/ui/chat/events.go |
Chat event handlers |
| 35 | internal/ui/chat/keybinds.go |
Chat keymap |
| 36 | internal/ui/chat/guilds_tree.go |
Guild/channel tree |
| 37 | internal/ui/chat/guildstate.go |
Guild expand persistence |
| 38 | internal/ui/chat/messages_list.go |
Message rendering + keybinds |
| 38a | internal/ui/chat/attachment_handler.go |
Attachment download/view/save |
| 38b | internal/ui/chat/embed_renderer.go |
Embed types + line wrapping |
| 38c | internal/ui/chat/url_extractor.go |
URL extraction from messages |
| 39 | internal/ui/chat/message_input.go |
Message input + mentions |
| 40 | internal/ui/chat/attachments_picker.go |
Attachment picker |
| 41 | internal/ui/chat/channels_picker.go |
Channel picker |
| 42 | internal/ui/chat/mentions_list.go |
Mentions autocomplete list |
| 43 | internal/ui/chat/util.go |
Picker config + humanJoin |
| 44 | internal/ui/login/model.go |
Login model |
| 45 | internal/ui/login/events.go |
Login events |
| 46 | internal/ui/login/keybinds.go |
Login keymap |
| 47 | internal/ui/login/token/model.go |
Token login form |
| 48 | internal/ui/login/token/events.go |
Token events |
| 49 | internal/ui/login/qr/model.go |
QR login model |
| 50 | internal/ui/login/qr/events.go |
QR login events + crypto |
| 51 | go.mod |
Module manifest |
| 52 | .github/workflows/ci.yml |
CI configuration |
| 53 | .gitignore |
Git ignore rules |
| 54 | README.md |
Project readme |
| 55 | CLAUDE.md |
Project context |
| 56 | internal/config/config.toml |
Default config (embedded) |