From 25061ea9a7feadc1d210886537e6b759080484dc Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Mon, 22 Dec 2025 21:30:57 -0800 Subject: [PATCH] chore: code health review - test fix and error comments (bd-9g1z, bd-ork0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove TestFindJSONLPathDefault from .test-skip (now passes) - Add explanatory comments to 24 ignored error locations in cmd/bd: - Cobra flag methods (MarkHidden, MarkRequired, MarkDeprecated) - Best-effort cleanup/close operations - Process signaling operations Part of code health review epic bd-tggf. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- .test-skip | 3 +-- cmd/bd/create.go | 2 +- cmd/bd/daemon_autostart.go | 8 ++++---- cmd/bd/daemon_lifecycle.go | 2 +- cmd/bd/daemon_lock.go | 8 ++++---- cmd/bd/duplicate.go | 4 ++-- cmd/bd/migrate.go | 6 +++--- cmd/bd/migrate_issues.go | 4 ++-- cmd/bd/mol_spawn.go | 2 +- cmd/bd/reset.go | 4 ++-- cmd/bd/show.go | 2 +- cmd/bd/snapshot_manager.go | 1 + cmd/bd/sync.go | 2 +- cmd/bd/tips.go | 2 +- docs/TESTING.md | 5 ----- 15 files changed, 25 insertions(+), 30 deletions(-) diff --git a/.test-skip b/.test-skip index a6483efb..ad552069 100644 --- a/.test-skip +++ b/.test-skip @@ -1,5 +1,4 @@ # Tests to skip due to known issues # Format: one test name per line (regex patterns supported) -# Issue #356: Expects wrong JSONL filename (issues.jsonl vs beads.jsonl) -TestFindJSONLPathDefault +# (no tests currently skipped) diff --git a/cmd/bd/create.go b/cmd/bd/create.go index 26d2f544..952394ab 100644 --- a/cmd/bd/create.go +++ b/cmd/bd/create.go @@ -399,7 +399,7 @@ func init() { registerCommonIssueFlags(createCmd) createCmd.Flags().StringSliceP("labels", "l", []string{}, "Labels (comma-separated)") createCmd.Flags().StringSlice("label", []string{}, "Alias for --labels") - _ = createCmd.Flags().MarkHidden("label") + _ = createCmd.Flags().MarkHidden("label") // Only fails if flag missing (caught in tests) createCmd.Flags().String("id", "", "Explicit issue ID (e.g., 'bd-42' for partitioning)") createCmd.Flags().String("parent", "", "Parent issue ID for hierarchical child (e.g., 'bd-a3f8e9')") createCmd.Flags().StringSlice("deps", []string{}, "Dependencies in format 'type:id' or 'id' (e.g., 'discovered-from:bd-20,blocks:bd-15' or 'bd-20')") diff --git a/cmd/bd/daemon_autostart.go b/cmd/bd/daemon_autostart.go index 245b98cb..d858c7d8 100644 --- a/cmd/bd/daemon_autostart.go +++ b/cmd/bd/daemon_autostart.go @@ -224,7 +224,7 @@ func acquireStartLock(lockPath, socketPath string) bool { } _, _ = fmt.Fprintf(lockFile, "%d\n", os.Getpid()) - _ = lockFile.Close() + _ = lockFile.Close() // Best-effort close during startup return true } @@ -257,9 +257,9 @@ func handleExistingSocket(socketPath string) bool { } debugLog("socket is stale, cleaning up") - _ = os.Remove(socketPath) + _ = os.Remove(socketPath) // Best-effort cleanup, file may not exist if pidFile != "" { - _ = os.Remove(pidFile) + _ = os.Remove(pidFile) // Best-effort cleanup, file may not exist } return false } @@ -353,7 +353,7 @@ func canDialSocket(socketPath string, timeout time.Duration) bool { if err != nil || client == nil { return false } - _ = client.Close() + _ = client.Close() // Best-effort close after health check return true } diff --git a/cmd/bd/daemon_lifecycle.go b/cmd/bd/daemon_lifecycle.go index 11981098..1ca5c669 100644 --- a/cmd/bd/daemon_lifecycle.go +++ b/cmd/bd/daemon_lifecycle.go @@ -303,7 +303,7 @@ func stopDaemon(pidFile string) { } // Clean up stale artifacts after forced kill - _ = os.Remove(pidFile) + _ = os.Remove(pidFile) // Best-effort cleanup, file may not exist // Also remove socket file if it exists if _, err := os.Stat(socketPath); err == nil { diff --git a/cmd/bd/daemon_lock.go b/cmd/bd/daemon_lock.go index 5be10a23..e6adab66 100644 --- a/cmd/bd/daemon_lock.go +++ b/cmd/bd/daemon_lock.go @@ -70,16 +70,16 @@ func acquireDaemonLock(beadsDir string, dbPath string) (*DaemonLock, error) { StartedAt: time.Now().UTC(), } - _ = f.Truncate(0) + _ = f.Truncate(0) // Clear file for fresh write (we hold lock) _, _ = f.Seek(0, 0) encoder := json.NewEncoder(f) encoder.SetIndent("", " ") - _ = encoder.Encode(lockInfo) - _ = f.Sync() + _ = encoder.Encode(lockInfo) // Write can't fail if Truncate succeeded + _ = f.Sync() // Best-effort sync to disk // Also write PID file for Windows compatibility (can't read locked files on Windows) pidFile := filepath.Join(beadsDir, "daemon.pid") - _ = os.WriteFile(pidFile, []byte(fmt.Sprintf("%d\n", os.Getpid())), 0600) + _ = os.WriteFile(pidFile, []byte(fmt.Sprintf("%d\n", os.Getpid())), 0600) // Best-effort PID write return &DaemonLock{file: f, path: lockPath}, nil } diff --git a/cmd/bd/duplicate.go b/cmd/bd/duplicate.go index 7d76a81f..03cc6899 100644 --- a/cmd/bd/duplicate.go +++ b/cmd/bd/duplicate.go @@ -49,11 +49,11 @@ var ( func init() { duplicateCmd.Flags().StringVar(&duplicateOf, "of", "", "Canonical issue ID (required)") - _ = duplicateCmd.MarkFlagRequired("of") + _ = duplicateCmd.MarkFlagRequired("of") // Only fails if flag missing (caught in tests) rootCmd.AddCommand(duplicateCmd) supersedeCmd.Flags().StringVar(&supersededWith, "with", "", "Replacement issue ID (required)") - _ = supersedeCmd.MarkFlagRequired("with") + _ = supersedeCmd.MarkFlagRequired("with") // Only fails if flag missing (caught in tests) rootCmd.AddCommand(supersedeCmd) } diff --git a/cmd/bd/migrate.go b/cmd/bd/migrate.go index b65179be..e06470f2 100644 --- a/cmd/bd/migrate.go +++ b/cmd/bd/migrate.go @@ -721,9 +721,9 @@ func cleanupWALFiles(dbPath string) { walPath := dbPath + "-wal" shmPath := dbPath + "-shm" - // Best effort - don't fail if these don't exist - _ = os.Remove(walPath) - _ = os.Remove(shmPath) + // Best effort cleanup - don't fail if these don't exist + _ = os.Remove(walPath) // WAL may not exist + _ = os.Remove(shmPath) // SHM may not exist } // handleInspect shows migration plan and database state for AI agent analysis diff --git a/cmd/bd/migrate_issues.go b/cmd/bd/migrate_issues.go index da83d37f..d29cd376 100644 --- a/cmd/bd/migrate_issues.go +++ b/cmd/bd/migrate_issues.go @@ -708,6 +708,6 @@ func init() { migrateIssuesCmd.Flags().Bool("strict", false, "Fail on orphaned dependencies or missing repos") migrateIssuesCmd.Flags().Bool("yes", false, "Skip confirmation prompt") - _ = migrateIssuesCmd.MarkFlagRequired("from") - _ = migrateIssuesCmd.MarkFlagRequired("to") + _ = migrateIssuesCmd.MarkFlagRequired("from") // Only fails if flag missing (caught in tests) + _ = migrateIssuesCmd.MarkFlagRequired("to") // Only fails if flag missing (caught in tests) } diff --git a/cmd/bd/mol_spawn.go b/cmd/bd/mol_spawn.go index b15567e6..f59369f6 100644 --- a/cmd/bd/mol_spawn.go +++ b/cmd/bd/mol_spawn.go @@ -256,7 +256,7 @@ func init() { molSpawnCmd.Flags().String("attach-type", types.BondTypeSequential, "Bond type for attachments: sequential, parallel, or conditional") molSpawnCmd.Flags().Bool("pour", false, "Create persistent mol in .beads/ (default: wisp in .beads-wisp/)") molSpawnCmd.Flags().Bool("persistent", false, "Deprecated: use --pour instead") - _ = molSpawnCmd.Flags().MarkDeprecated("persistent", "use --pour instead") + _ = molSpawnCmd.Flags().MarkDeprecated("persistent", "use --pour instead") // Only fails if flag missing molCmd.AddCommand(molSpawnCmd) } diff --git a/cmd/bd/reset.go b/cmd/bd/reset.go index 0d9611dc..24e6fc4f 100644 --- a/cmd/bd/reset.go +++ b/cmd/bd/reset.go @@ -322,7 +322,7 @@ func stopDaemonQuiet(pidFile string) { return } - _ = sendStopSignal(process) + _ = sendStopSignal(process) // Best-effort graceful stop // Wait for daemon to stop gracefully for i := 0; i < daemonShutdownAttempts; i++ { @@ -333,7 +333,7 @@ func stopDaemonQuiet(pidFile string) { } // Force kill if still running - _ = process.Kill() + _ = process.Kill() // Best-effort force kill, process may have already exited } func removeGitattributesEntry() error { diff --git a/cmd/bd/show.go b/cmd/bd/show.go index 2d136873..9260727e 100644 --- a/cmd/bd/show.go +++ b/cmd/bd/show.go @@ -1394,7 +1394,7 @@ func init() { registerCommonIssueFlags(updateCmd) updateCmd.Flags().String("notes", "", "Additional notes") updateCmd.Flags().String("acceptance-criteria", "", "DEPRECATED: use --acceptance") - _ = updateCmd.Flags().MarkHidden("acceptance-criteria") + _ = updateCmd.Flags().MarkHidden("acceptance-criteria") // Only fails if flag missing (caught in tests) updateCmd.Flags().IntP("estimate", "e", 0, "Time estimate in minutes (e.g., 60 for 1 hour)") updateCmd.Flags().StringSlice("add-label", nil, "Add labels (repeatable)") updateCmd.Flags().StringSlice("remove-label", nil, "Remove labels (repeatable)") diff --git a/cmd/bd/snapshot_manager.go b/cmd/bd/snapshot_manager.go index 9ca19cc3..f7de8199 100644 --- a/cmd/bd/snapshot_manager.go +++ b/cmd/bd/snapshot_manager.go @@ -192,6 +192,7 @@ func (sm *SnapshotManager) Cleanup() error { basePath, leftPath := sm.getSnapshotPaths() baseMetaPath, leftMetaPath := sm.getSnapshotMetadataPaths() + // Best-effort cleanup of snapshot files (may not exist) _ = os.Remove(basePath) _ = os.Remove(leftPath) _ = os.Remove(baseMetaPath) diff --git a/cmd/bd/sync.go b/cmd/bd/sync.go index 35307cd6..23d9d8b1 100644 --- a/cmd/bd/sync.go +++ b/cmd/bd/sync.go @@ -1402,7 +1402,7 @@ func exportToJSONL(ctx context.Context, jsonlPath string) error { exportedIDs = append(exportedIDs, issue.ID) } - // Close temp file before rename + // Close temp file before rename (error checked implicitly by Rename success) _ = tempFile.Close() // Atomic replace diff --git a/cmd/bd/tips.go b/cmd/bd/tips.go index 19659404..0c486fd0 100644 --- a/cmd/bd/tips.go +++ b/cmd/bd/tips.go @@ -154,7 +154,7 @@ func getLastShown(store storage.Storage, tipID string) time.Time { func recordTipShown(store storage.Storage, tipID string) { key := fmt.Sprintf("tip_%s_last_shown", tipID) value := time.Now().Format(time.RFC3339) - _ = store.SetMetadata(context.Background(), key, value) + _ = store.SetMetadata(context.Background(), key, value) // Non-critical metadata, ok to fail silently } // InjectTip adds a dynamic tip to the registry at runtime. diff --git a/docs/TESTING.md b/docs/TESTING.md index 28e279f5..fa63dd66 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -67,11 +67,6 @@ Tests in `.test-skip` are automatically skipped. Current broken tests: - Issue: Database deadlock, hangs for 5 minutes - Impact: Makes test suite extremely slow -2. **TestFindJSONLPathDefault** (GH #356) - - Location: `internal/beads/beads_test.go:175` - - Issue: Expects `issues.jsonl` but code returns `beads.jsonl` - - Impact: Assertion failure - ## For Claude Code / AI Agents When running tests during development: