From ddab26315f43f5e857e79cb172c0380fc93100e5 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Fri, 31 Oct 2025 15:12:01 -0700 Subject: [PATCH] Remove dead code found by deadcode analyzer - Removed 5 unreachable functions (~200 LOC) - computeIssueContentHash, shouldSkipExport from autoflush.go - addDependencyUnchecked, removeDependencyIfExists from dependencies.go - isUniqueConstraintError alias from util.go - All tests still pass - Closes bd-7c5915ae --- .beads/beads.jsonl | 2 +- cmd/bd/autoflush.go | 49 -------- internal/storage/sqlite/dependencies.go | 156 ------------------------ internal/storage/sqlite/util.go | 5 +- 4 files changed, 2 insertions(+), 210 deletions(-) diff --git a/.beads/beads.jsonl b/.beads/beads.jsonl index 82e6308d..ed57e0cc 100644 --- a/.beads/beads.jsonl +++ b/.beads/beads.jsonl @@ -61,7 +61,7 @@ {"id":"bd-7a00c94e","content_hash":"b31566a4b2a84db7d24364492e8ac6ebfa1f5fc27fe270fbd58b27e17218c9c4","title":"Rapid 2","description":"","status":"open","priority":3,"issue_type":"task","created_at":"2025-10-29T19:11:57.430725-07:00","updated_at":"2025-10-30T17:12:58.189251-07:00"} {"id":"bd-7a2b58fc","content_hash":"e5f3cb5dc86ba8925237e37359f796b806fb302a148ea8c5c017ee014a0d425a","title":"Implement clone-scoped ID allocation to prevent N-way collisions","description":"## Problem\nCurrent ID allocation uses per-clone atomic counters (issue_counters table) that sync based on local database state. In N-way collision scenarios:\n- Clone B sees {test-1} locally, allocates test-2\n- Clone D sees {test-1, test-2, test-3} locally, allocates test-4\n- When same content gets assigned test-2 and test-4, convergence fails\n\nRoot cause: Each clone independently allocates IDs without global coordination, leading to overlapping assignments for the same content.\n\n## Solution\nAdd clone UUID to ID allocation to make every ID globally unique:\n\n**Current format:** `test-1`, `test-2`, `test-3`\n**New format:** `test-1-a7b3`, `test-2-a7b3`, `test-3-c4d9`\n\nWhere suffix is first 4 chars of clone UUID.\n\n## Implementation\n\n### 1. Add clone_identity table\n```sql\nCREATE TABLE clone_identity (\n clone_uuid TEXT PRIMARY KEY,\n created_at DATETIME DEFAULT CURRENT_TIMESTAMP\n);\n```\n\n### 2. Modify getNextIDForPrefix()\n```go\nfunc (s *SQLiteStorage) getNextIDForPrefix(ctx context.Context, prefix string) (string, error) {\n cloneUUID := s.getOrCreateCloneUUID(ctx)\n shortUUID := cloneUUID[:4]\n \n nextNum := s.getNextCounterForPrefix(ctx, prefix)\n return fmt.Sprintf(\"%s-%d-%s\", prefix, nextNum, shortUUID), nil\n}\n```\n\n### 3. Update ID parsing logic\nAll places that parse IDs (utils.ExtractIssueNumber, etc.) need to handle new format.\n\n### 4. Migration strategy\n- Existing IDs remain unchanged (no suffix)\n- New IDs get clone suffix automatically\n- Display layer can hide suffix in UI: `bd-cb64c226.3-a7b3` → `#42`\n\n## Benefits\n- **Zero collision risk**: Same content in different clones gets different IDs\n- **Maintains readability**: Still sequential numbering within clone\n- **No coordination needed**: Works offline, no central authority\n- **Scales to 100+ clones**: 4-char hex = 65,536 unique clones\n\n## Concerns\n- ID format change may break existing integrations\n- Need migration path for existing databases\n- Display logic needs update to hide/show suffixes appropriately\n\n## Success Criteria\n- 10+ clone collision test passes without failures\n- Existing issues continue to work (backward compatibility)\n- Documentation updated with new ID format\n- Migration guide for v1.x → v2.x\n\n## Timeline\nMedium-term (v1.1-v1.2), 2-3 weeks implementation\n\n## References\n- Related to bd-0dcea000 (immediate fix)\n- See beads_nway_test.go for failing N-way tests","status":"open","priority":2,"issue_type":"feature","created_at":"2025-10-29T20:02:47.952447-07:00","updated_at":"2025-10-30T17:12:58.194389-07:00"} {"id":"bd-7bbc4e6a","title":"Add MCP server functions for repair commands","description":"Expose new repair commands via MCP server for agent access:\n\nFunctions to add:\n- beads_repair_deps()\n- beads_detect_pollution()\n- beads_validate()\n- beads_resolve_conflicts() (when implemented)\n\nUpdate integrations/beads-mcp/src/beads_mcp/server.py\n\nSee repair_commands.md lines 803-884 for design.","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-28T19:37:55.72639-07:00","updated_at":"2025-10-30T17:12:58.179948-07:00"} -{"id":"bd-7c5915ae","content_hash":"8407a18ee38e96f92e7c7afde2f39b3df6fad409ccd5080243925d8a05fc85c1","title":"Run final validation and cleanup checks","description":"Final validation pass to ensure all cleanup objectives met and no regressions introduced.\n\nValidation checklist:\n1. Dead code verification: `go run golang.org/x/tools/cmd/deadcode@latest -test ./...`\n2. Test coverage: `go test -cover ./...`\n3. Build verification: `go build ./cmd/bd/`\n4. Linting: `golangci-lint run`\n5. Integration tests\n6. Metrics verification\n7. Git clean check\n\nFinal metrics to report:\n- LOC removed: ~____\n- Files deleted: ____\n- Files created: ____\n- Test coverage: ____%\n- Build time: ____ (before/after)\n- Test run time: ____ (before/after)\n\nImpact: Confirms all cleanup objectives achieved successfully","acceptance_criteria":"- Zero unreachable functions per deadcode analyzer\n- All tests pass: `go test ./...`\n- Test coverage maintained or improved\n- Builds cleanly: `go build ./...`\n- Linting shows improvements\n- Integration tests all pass\n- LOC reduction target achieved (~2,500 LOC)\n- No unintended behavior changes\n- Git commit messages document all changes","status":"open","priority":1,"issue_type":"task","created_at":"2025-10-29T20:49:49.131575-07:00","updated_at":"2025-10-30T17:12:58.222155-07:00"} +{"id":"bd-7c5915ae","content_hash":"7b4813f1ea1996fb65748a3610937bc63b26202725fcf4d42aac71b430f34e69","title":"Run final validation and cleanup checks","description":"Final validation pass to ensure all cleanup objectives met and no regressions introduced.\n\nValidation checklist:\n1. Dead code verification: `go run golang.org/x/tools/cmd/deadcode@latest -test ./...`\n2. Test coverage: `go test -cover ./...`\n3. Build verification: `go build ./cmd/bd/`\n4. Linting: `golangci-lint run`\n5. Integration tests\n6. Metrics verification\n7. Git clean check\n\nFinal metrics to report:\n- LOC removed: ~____\n- Files deleted: ____\n- Files created: ____\n- Test coverage: ____%\n- Build time: ____ (before/after)\n- Test run time: ____ (before/after)\n\nImpact: Confirms all cleanup objectives achieved successfully","acceptance_criteria":"- Zero unreachable functions per deadcode analyzer\n- All tests pass: `go test ./...`\n- Test coverage maintained or improved\n- Builds cleanly: `go build ./...`\n- Linting shows improvements\n- Integration tests all pass\n- LOC reduction target achieved (~2,500 LOC)\n- No unintended behavior changes\n- Git commit messages document all changes","status":"in_progress","priority":1,"issue_type":"task","created_at":"2025-10-29T20:49:49.131575-07:00","updated_at":"2025-10-31T15:09:32.790228-07:00"} {"id":"bd-7c831c51","content_hash":"192d94c432595c1051f254a25bb6325e9b98ceccfb369dc43e0619c27016feae","title":"Run final validation and cleanup checks","description":"Final validation pass to ensure all cleanup objectives met and no regressions introduced.\n\nValidation checklist:\n1. Dead code verification: `go run golang.org/x/tools/cmd/deadcode@latest -test ./...`\n2. Test coverage: `go test -cover ./...`\n3. Build verification: `go build ./cmd/bd/`\n4. Linting: `golangci-lint run`\n5. Integration tests\n6. Metrics verification\n7. Git clean check\n\nFinal metrics to report:\n- LOC removed: ~____\n- Files deleted: ____\n- Files created: ____\n- Test coverage: ____%\n- Build time: ____ (before/after)\n- Test run time: ____ (before/after)\n\nImpact: Confirms all cleanup objectives achieved successfully","acceptance_criteria":"- Zero unreachable functions per deadcode analyzer\n- All tests pass: `go test ./...`\n- Test coverage maintained or improved\n- Builds cleanly: `go build ./...`\n- Linting shows improvements\n- Integration tests all pass\n- LOC reduction target achieved (~2,500 LOC)\n- No unintended behavior changes\n- Git commit messages document all changes","notes":"## Validation Results\n\n**Dead Code:** ✅ Found and removed 1 unreachable function (`DroppedEventsCount`) \n**Tests:** ✅ All pass \n**Coverage:** \n- Main: 39.6%\n- cmd/bd: 20.2%\n- Created follow-up issues (bd-85487065 through bd-bc2c6191) to improve coverage\n \n**Build:** ✅ Clean \n**Linting:** 73 issues (up from 34 baseline) \n- Increase due to unused functions from refactoring\n- Need cleanup in separate issue\n \n**Integration Tests:** ✅ All pass \n**Metrics:** 56,464 LOC across 193 Go files \n**Git:** 2 files modified (deadcode fix + auto-synced JSONL)\n\n## Follow-up Issues Created\n- bd-85487065: Add tests for internal/autoimport (0% coverage)\n- bd-0dcea000: Add tests for internal/importer (0% coverage)\n- bd-4d7fca8a: Add tests for internal/utils (0% coverage)\n- bd-6221bdcd: Improve cmd/bd coverage (20.2% -\u003e target higher)\n- bd-bc2c6191: Improve internal/daemon coverage (22.5% -\u003e target higher)","status":"closed","priority":1,"issue_type":"task","created_at":"2025-10-29T20:02:47.956276-07:00","updated_at":"2025-10-30T17:12:58.193468-07:00","closed_at":"2025-10-29T14:19:35.095553-07:00"} {"id":"bd-7da9437e","content_hash":"c00bf7c9fe41b90f1bd3cd1e7cf6938ca4e42f076ce45c2a3d836db97c883fc4","title":"Latency test","description":"","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-29T15:28:52.729923-07:00","updated_at":"2025-10-31T12:00:43.184758-07:00","closed_at":"2025-10-31T12:00:43.184758-07:00"} {"id":"bd-7e0d6660","content_hash":"84f212d47832be4670333dc0148e3de158ca3a2dc7cb68b992f8536409272cfb","title":"Handle unchecked errors (errcheck - 683 issues)","description":"683 unchecked error returns, mostly in tests (Close, Rollback, RemoveAll). Many already excluded in config but still showing up.","design":"Review .golangci.yml exclude-rules. Most defer Close/Rollback errors in tests can be ignored. Add systematic exclusions or explicit _ = assignments where appropriate.","notes":"Fixed all errcheck warnings in production code:\n- Enabled errcheck linter (was disabled)\n- Set tests: false in .golangci.yml to focus on production code\n- Fixed 27 total errors in production code using Oracle guidance:\n * Database patterns: defer func() { _ = rows.Close() }() and defer func() { _ = tx.Rollback() }()\n * Best-effort closers: _ = store.Close(), _ = client.Close()\n * Proper error handling for file writes, fmt.Scanln(), os.Remove()\n- All tests pass\n- Only 2 \"unused\" linter warnings remain (not errcheck)","status":"closed","priority":3,"issue_type":"task","created_at":"2025-10-27T23:20:10.392336-07:00","updated_at":"2025-10-30T17:12:58.215288-07:00","closed_at":"2025-10-27T23:05:31.945328-07:00"} diff --git a/cmd/bd/autoflush.go b/cmd/bd/autoflush.go index 07ae8e1a..0d63b749 100644 --- a/cmd/bd/autoflush.go +++ b/cmd/bd/autoflush.go @@ -390,55 +390,6 @@ func clearAutoFlushState() { // // Error handling: Returns error on any failure. Cleanup is guaranteed via defer. // Thread-safe: No shared state access. Safe to call from multiple goroutines. -// computeIssueContentHash computes a SHA256 hash of an issue's content, excluding timestamps. -// This is used for detecting timestamp-only changes during export deduplication (bd-159). -func computeIssueContentHash(issue *types.Issue) (string, error) { - // Clone issue and zero out timestamps to exclude them from hash - normalized := *issue - normalized.CreatedAt = time.Time{} - normalized.UpdatedAt = time.Time{} - - // Also zero out ClosedAt if present - if normalized.ClosedAt != nil { - zeroTime := time.Time{} - normalized.ClosedAt = &zeroTime - } - - // Serialize to JSON - data, err := json.Marshal(normalized) - if err != nil { - return "", err - } - - // SHA256 hash - hash := sha256.Sum256(data) - return hex.EncodeToString(hash[:]), nil -} - -// shouldSkipExport checks if an issue should be skipped during export because -// it only has timestamp changes (no actual content changes) (bd-159). -func shouldSkipExport(ctx context.Context, issue *types.Issue) (bool, error) { - // Get the stored hash from export_hashes table (last exported state) - storedHash, err := store.GetExportHash(ctx, issue.ID) - if err != nil { - return false, err - } - - // If no hash stored, we must export (first export) - if storedHash == "" { - return false, nil - } - - // Compute current hash - currentHash, err := computeIssueContentHash(issue) - if err != nil { - return false, err - } - - // If hashes match, only timestamps changed - skip export - return currentHash == storedHash, nil -} - // validateJSONLIntegrity checks if JSONL file hash matches stored hash. // If mismatch detected, clears export_hashes and logs warning (bd-160). func validateJSONLIntegrity(ctx context.Context, jsonlPath string) error { diff --git a/internal/storage/sqlite/dependencies.go b/internal/storage/sqlite/dependencies.go index 3ae79540..b074df70 100644 --- a/internal/storage/sqlite/dependencies.go +++ b/internal/storage/sqlite/dependencies.go @@ -158,119 +158,6 @@ func (s *SQLiteStorage) AddDependency(ctx context.Context, dep *types.Dependency return tx.Commit() } -// addDependencyUnchecked adds a dependency with minimal validation, used during -// import/remap operations where we're preserving existing dependencies with new IDs. -// Skips semantic validation (parent-child direction) but keeps essential checks: -// - Issue existence validation -// - Self-dependency prevention -// - Cycle detection -func (s *SQLiteStorage) addDependencyUnchecked(ctx context.Context, dep *types.Dependency, actor string) error { - // Validate dependency type - if !dep.Type.IsValid() { - return fmt.Errorf("invalid dependency type: %s", dep.Type) - } - - // Validate that both issues exist - issueExists, err := s.GetIssue(ctx, dep.IssueID) - if err != nil { - return fmt.Errorf("failed to check issue %s: %w", dep.IssueID, err) - } - if issueExists == nil { - return fmt.Errorf("issue %s not found", dep.IssueID) - } - - dependsOnExists, err := s.GetIssue(ctx, dep.DependsOnID) - if err != nil { - return fmt.Errorf("failed to check dependency %s: %w", dep.DependsOnID, err) - } - if dependsOnExists == nil { - return fmt.Errorf("dependency target %s not found", dep.DependsOnID) - } - - // Prevent self-dependency - if dep.IssueID == dep.DependsOnID { - return fmt.Errorf("issue cannot depend on itself") - } - - // NOTE: We skip parent-child direction validation here because during import/remap, - // we're just updating IDs on existing dependencies that were already validated. - - if dep.CreatedAt.IsZero() { - dep.CreatedAt = time.Now() - } - if dep.CreatedBy == "" { - dep.CreatedBy = actor - } - - tx, err := s.db.BeginTx(ctx, nil) - if err != nil { - return fmt.Errorf("failed to begin transaction: %w", err) - } - defer func() { _ = tx.Rollback() }() - - // Cycle detection (same as AddDependency) - var cycleExists bool - err = tx.QueryRowContext(ctx, ` - WITH RECURSIVE paths AS ( - SELECT - issue_id, - depends_on_id, - 1 as depth - FROM dependencies - WHERE issue_id = ? - - UNION ALL - - SELECT - d.issue_id, - d.depends_on_id, - p.depth + 1 - FROM dependencies d - JOIN paths p ON d.issue_id = p.depends_on_id - WHERE p.depth < ? - ) - SELECT EXISTS( - SELECT 1 FROM paths - WHERE depends_on_id = ? - ) - `, dep.DependsOnID, maxDependencyDepth, dep.IssueID).Scan(&cycleExists) - - if err != nil { - return fmt.Errorf("failed to check for cycles: %w", err) - } - - if cycleExists { - return fmt.Errorf("cannot add dependency: would create a cycle (%s → %s → ... → %s)", - dep.IssueID, dep.DependsOnID, dep.IssueID) - } - - // Insert dependency - _, err = tx.ExecContext(ctx, ` - INSERT INTO dependencies (issue_id, depends_on_id, type, created_at, created_by) - VALUES (?, ?, ?, ?, ?) - `, dep.IssueID, dep.DependsOnID, dep.Type, dep.CreatedAt, dep.CreatedBy) - if err != nil { - return fmt.Errorf("failed to add dependency: %w", err) - } - - // Record event - _, err = tx.ExecContext(ctx, ` - INSERT INTO events (issue_id, event_type, actor, comment) - VALUES (?, ?, ?, ?) - `, dep.IssueID, types.EventDependencyAdded, actor, - fmt.Sprintf("Added dependency: %s %s %s", dep.IssueID, dep.Type, dep.DependsOnID)) - if err != nil { - return fmt.Errorf("failed to record event: %w", err) - } - - // Mark both issues as dirty - if err := markIssuesDirtyTx(ctx, tx, []string{dep.IssueID, dep.DependsOnID}); err != nil { - return err - } - - return tx.Commit() -} - // RemoveDependency removes a dependency func (s *SQLiteStorage) RemoveDependency(ctx context.Context, issueID, dependsOnID string, actor string) error { tx, err := s.db.BeginTx(ctx, nil) @@ -312,49 +199,6 @@ func (s *SQLiteStorage) RemoveDependency(ctx context.Context, issueID, dependsOn return tx.Commit() } -// removeDependencyIfExists removes a dependency, returning nil if it doesn't exist -// This is useful during remapping where dependencies may have been already removed -func (s *SQLiteStorage) removeDependencyIfExists(ctx context.Context, issueID, dependsOnID string, actor string) error { - tx, err := s.db.BeginTx(ctx, nil) - if err != nil { - return fmt.Errorf("failed to begin transaction: %w", err) - } - defer func() { _ = tx.Rollback() }() - - result, err := tx.ExecContext(ctx, ` - DELETE FROM dependencies WHERE issue_id = ? AND depends_on_id = ? - `, issueID, dependsOnID) - if err != nil { - return fmt.Errorf("failed to remove dependency: %w", err) - } - - // Check if dependency existed - if not, that's okay, just skip the event - rowsAffected, err := result.RowsAffected() - if err != nil { - return fmt.Errorf("failed to check rows affected: %w", err) - } - if rowsAffected == 0 { - // Dependency didn't exist, nothing to do - return nil - } - - _, err = tx.ExecContext(ctx, ` - INSERT INTO events (issue_id, event_type, actor, comment) - VALUES (?, ?, ?, ?) - `, issueID, types.EventDependencyRemoved, actor, - fmt.Sprintf("Removed dependency on %s", dependsOnID)) - if err != nil { - return fmt.Errorf("failed to record event: %w", err) - } - - // Mark both issues as dirty for incremental export - if err := markIssuesDirtyTx(ctx, tx, []string{issueID, dependsOnID}); err != nil { - return err - } - - return tx.Commit() -} - // GetDependencies returns issues that this issue depends on func (s *SQLiteStorage) GetDependencies(ctx context.Context, issueID string) ([]*types.Issue, error) { rows, err := s.db.QueryContext(ctx, ` diff --git a/internal/storage/sqlite/util.go b/internal/storage/sqlite/util.go index 46807dee..2a8f3e79 100644 --- a/internal/storage/sqlite/util.go +++ b/internal/storage/sqlite/util.go @@ -48,7 +48,4 @@ func IsUniqueConstraintError(err error) bool { return strings.Contains(err.Error(), "UNIQUE constraint failed") } -// isUniqueConstraintError is an alias for IsUniqueConstraintError for internal use -func isUniqueConstraintError(err error) bool { - return IsUniqueConstraintError(err) -} +