From aa38f686706406113b958a1244ad037e60dcf564 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Mon, 27 Oct 2025 18:42:56 -0700 Subject: [PATCH] Update LINTING.md with current baseline of 34 issues (bd-45) --- LINTING.md | 68 ++++++++++++++++++------------------------------------ 1 file changed, 22 insertions(+), 46 deletions(-) diff --git a/LINTING.md b/LINTING.md index 7f70f111..64ec7d55 100644 --- a/LINTING.md +++ b/LINTING.md @@ -4,13 +4,13 @@ This document explains our approach to `golangci-lint` warnings in this codebase ## Current Status -Running `golangci-lint run ./...` currently reports ~200 "issues". However, these are not actual code quality problems - they are false positives or intentional patterns that reflect idiomatic Go practice. +Running `golangci-lint run ./...` currently reports **34 issues** as of Oct 27, 2025. These are not actual code quality problems - they are false positives or intentional patterns that reflect idiomatic Go practice. -**Note**: The count increased from ~100 to ~200 between Oct 12-14, 2025, due to significant test coverage additions for collision resolution (1100+ lines) and auto-flush features (300+ lines). All new warnings follow the same idiomatic patterns documented below. +**Historical note**: The count was ~200 before extensive cleanup in October 2025. The remaining issues represent the acceptable baseline that doesn't warrant fixing. ## Issue Breakdown -### errcheck (159 issues) +### errcheck (24 issues) **Pattern**: Unchecked errors from `defer` cleanup operations **Status**: Intentional and idiomatic @@ -29,59 +29,35 @@ defer os.RemoveAll(tmpDir) // in tests Fixing these would add noise without improving code quality. The critical cleanup operations (where errors matter) are already checked explicitly. -### revive (21 issues) +### gosec (10 issues) -**Pattern 1**: Unused parameters in Cobra command handlers (18 issues) -**Status**: Required by interface +**Pattern 1**: G204 - Subprocess launched with variable (3 issues) +**Status**: Intentional - launching editor and git commands with user-specified paths Examples: -```go -Run: func(cmd *cobra.Command, args []string) { - // cmd or args may not be used in every handler -} -``` +- Launching `$EDITOR` for issue editing +- Executing git commands +- Running bd daemon binary -**Rationale**: Cobra requires this exact function signature. Renaming to `_` would make the code less clear when parameters *are* used. - -**Pattern 2**: Package naming (3 issues) -- `package types` - Clear and appropriate for a types package -- `SQLiteStorage` - Intentional; `sqlite.Storage` would be confusing with the interface -- Blank import comment - Required for database driver registration - -### gosec (19 issues) - -**Pattern 1**: G201 - SQL string formatting (6 issues) -**Status**: False positive - all SQL is validated - -All dynamic SQL construction uses: -- Validated field names via allowlist (see `allowedUpdateFields` in sqlite.go:197) -- Parameterized queries for all values -- Safe string building for clauses like ORDER BY and LIMIT - -**Pattern 2**: G304 - File inclusion via variable (11 issues) -**Status**: Intended feature - user-specified file paths for import/export/test fixtures +**Pattern 2**: G304 - File inclusion via variable (3 issues) +**Status**: Intended feature - user-specified file paths for import/export All file paths are either: - User-provided CLI arguments (expected for import/export commands) - Test fixtures in controlled test environments -- Validated paths with security checks (e.g., markdown.go uses validateMarkdownPath) +- Validated paths with security checks -**Pattern 3**: G301 - Directory permissions (2 issues) -**Status**: Acceptable - 0755 is reasonable for database directories +**Pattern 3**: G301/G302/G306 - File permissions (3 issues) +**Status**: Acceptable for user-facing database files -### gocyclo (1 issue) +- G301: 0755 for database directories (allows other users to read) +- G302: 0644 for JSONL files (version controlled, needs to be readable) +- G306: 0644 for new JSONL files (consistency with existing files) -**Pattern**: High cyclomatic complexity in `TestExportImport` (31) -**Status**: Acceptable +**Pattern 4**: G115 - Integer overflow conversion (1 issue) +**Status**: False positive - bounded by max retry count -This comprehensive integration test covers multiple scenarios (export, import, filters, updates). The complexity comes from thorough test coverage, not production code. Splitting would reduce readability. - -### goconst (2 issues) - -**Pattern**: Repeated string constants in tests -**Status**: Acceptable - -Repeated test strings like `"test-user"` and file paths appear multiple times. Extracting these to constants would not improve test readability or maintainability. +The exponential backoff calculation is bounded by a small retry counter, making overflow impossible in practice. ## golangci-lint Configuration Challenges @@ -94,10 +70,10 @@ This appears to be a known limitation of golangci-lint's configuration system. ## Recommendation -**For contributors**: Don't be alarmed by the lint warnings. The code quality is high. +**For contributors**: Don't be alarmed by the 34 lint warnings. The code quality is high. **For code review**: Focus on: -- New issues introduced by changes (not the baseline ~200) +- New issues introduced by changes (not the baseline 34) - Actual logic errors - Missing error checks on critical operations (file writes, database commits) - Security concerns beyond gosec's false positives