From 9c3ab7fba960d5d71eb5f9161640e1acc29df820 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Thu, 6 Nov 2025 19:42:01 -0800 Subject: [PATCH] Document -short flag, update linting baseline, clarify bd edit MCP exclusion - Enhanced CONTRIBUTING.md with -short flag usage guidance - Updated LINTING.md: 22 issues baseline (down from 34) - Added MCP exclusion note for bd edit command in AGENTS.md Closes bd-iov0, bd-aec5439f, bd-fd8753d9 --- AGENTS.md | 2 ++ CONTRIBUTING.md | 16 ++++++++++++++-- docs/LINTING.md | 35 ++++++++++++++++++++++++++--------- 3 files changed, 42 insertions(+), 11 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 98107647..934ad7b3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -172,6 +172,8 @@ bd update [...] --status in_progress --json bd update [...] --priority 1 --json # Edit issue fields in $EDITOR (HUMANS ONLY - not for agents) +# NOTE: This command is intentionally NOT exposed via the MCP server +# Agents should use 'bd update' with field-specific parameters instead bd edit # Edit description bd edit --title # Edit title bd edit --design # Edit design notes diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2148f7b6..eddc79b6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -135,16 +135,28 @@ Slow tests use `testing.Short()` to skip when `-short` flag is present. ### Running Tests ```bash -# Fast tests (recommended for development) +# Fast tests (recommended for development - skips slow tests) +# Use this for rapid iteration during development go test -short ./... -# Full test suite (before committing) +# Full test suite (before committing - includes all tests) +# Run this before pushing to ensure nothing breaks go test ./... # With race detection and coverage go test -race -coverprofile=coverage.out ./... ``` +**When to use `-short`:** +- During active development for fast feedback loops +- When making small changes that don't affect integration points +- When you want to quickly verify unit tests pass + +**When to use full test suite:** +- Before committing and pushing changes +- After modifying git operations or multi-clone scenarios +- When preparing a pull request + ### Writing Tests - Write table-driven tests when testing multiple scenarios diff --git a/docs/LINTING.md b/docs/LINTING.md index 64ec7d55..556bce32 100644 --- a/docs/LINTING.md +++ b/docs/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 **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. +Running `golangci-lint run ./...` currently reports **22 issues** as of Nov 6, 2025. These are not actual code quality problems - they are false positives or intentional patterns that reflect idiomatic Go practice. -**Historical note**: The count was ~200 before extensive cleanup in October 2025. The remaining issues represent the acceptable baseline that doesn't warrant fixing. +**Historical note**: The count was ~200 before extensive cleanup in October 2025, reduced to 34 by Oct 27, and now 22 after internal/daemonrunner removal. The remaining issues represent the acceptable baseline that doesn't warrant fixing. ## Issue Breakdown -### errcheck (24 issues) +### errcheck (4 issues) **Pattern**: Unchecked errors from `defer` cleanup operations **Status**: Intentional and idiomatic @@ -29,7 +29,7 @@ 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. -### gosec (10 issues) +### gosec (12 issues) **Pattern 1**: G204 - Subprocess launched with variable (3 issues) **Status**: Intentional - launching editor and git commands with user-specified paths @@ -54,10 +54,27 @@ All file paths are either: - G302: 0644 for JSONL files (version controlled, needs to be readable) - G306: 0644 for new JSONL files (consistency with existing files) -**Pattern 4**: G115 - Integer overflow conversion (1 issue) -**Status**: False positive - bounded by max retry count +**Pattern 4**: G201/G202 - SQL string formatting/concatenation (3 issues) +**Status**: Safe - using placeholders and bounded queries -The exponential backoff calculation is bounded by a small retry counter, making overflow impossible in practice. +All SQL concatenation uses proper placeholders and is bounded by controlled input (issue ID lists). + +### misspell (3 issues) + +**Pattern**: British vs American spelling - `cancelled` vs `canceled` +**Status**: Acceptable spelling variation + +The codebase uses "cancelled" (British spelling) in user-facing messages. Both spellings are correct. + +### unparam (4 issues) + +**Pattern**: Function parameters or return values that are always the same +**Status**: Interface compliance and future-proofing + +These functions maintain consistent signatures for: +- Interface implementations +- Future extensibility +- Code clarity and documentation ## golangci-lint Configuration Challenges @@ -70,10 +87,10 @@ This appears to be a known limitation of golangci-lint's configuration system. ## Recommendation -**For contributors**: Don't be alarmed by the 34 lint warnings. The code quality is high. +**For contributors**: Don't be alarmed by the 22 lint warnings. The code quality is high. **For code review**: Focus on: -- New issues introduced by changes (not the baseline 34) +- New issues introduced by changes (not the baseline 22) - Actual logic errors - Missing error checks on critical operations (file writes, database commits) - Security concerns beyond gosec's false positives