fix: address CI lint errors (gosec, errcheck, unparam, duplicate tests) (#730)

* fix: address CI lint errors (gosec, errcheck, unparam, duplicate tests) - Remove duplicate TestHandleDelete_DryRun and TestHandleDelete_PartialSuccess from server_mutations_test.go (already defined in server_delete_test.go) - Add nolint:gosec comments for exec.CommandContext calls in sync_branch.go (variables come from trusted config/git sources) - Fix gosec G304/G306 in yaml_config.go (file read/write permissions) - Fix errcheck in mol_run.go (templateStore.Close) - Add nolint:unparam for updateYamlKey error return * fix: add remaining nolint:gosec comments for exec.CommandContext calls - sync_branch.go: diffCmd, logCmd (dry-run), commitCmd, pushCmd, remoteCmd - sync_check.go: checkLocalCmd * fix: add more nolint:gosec comments for exec.CommandContext calls - sync_branch.go: pullCmd - sync_check.go: localRefCmd, remoteRefCmd, aheadCmd - sync_import.go: checkoutCmd * fix: add final nolint:gosec comments for exec.CommandContext calls - sync_check.go: behindCmd - sync_import.go: fetchCmd --------- Co-authored-by: Charles P. Cross <cpdata@users.noreply.github.com>
2025-12-24 15:35:32 -05:00
parent 177ee3265e
commit 8676c41c18
6 changed files with 25 additions and 200 deletions
--- a/cmd/bd/mol_run.go
+++ b/cmd/bd/mol_run.go
@@ -94,7 +94,7 @@ func runMolRun(cmd *cobra.Command, args []string) {
 			fmt.Fprintf(os.Stderr, "Error opening template database %s: %v\n", templateDB, err)
 			os.Exit(1)
 		}
-		defer templateStore.Close()
+		defer func() { _ = templateStore.Close() }()
 	}

 	// Resolve molecule ID from template store
--- a/cmd/bd/sync_branch.go
+++ b/cmd/bd/sync_branch.go
@@ -58,7 +58,7 @@ func showSyncStatus(ctx context.Context) error {
 	}

 	// Check if sync branch exists
-	checkCmd := exec.CommandContext(ctx, "git", "show-ref", "--verify", "--quiet", "refs/heads/"+syncBranch)
+	checkCmd := exec.CommandContext(ctx, "git", "show-ref", "--verify", "--quiet", "refs/heads/"+syncBranch) //nolint:gosec // syncBranch from config
 	if err := checkCmd.Run(); err != nil {
 		return fmt.Errorf("sync branch '%s' does not exist", syncBranch)
 	}
@@ -68,7 +68,7 @@ func showSyncStatus(ctx context.Context) error {

 	// Show commit diff
 	fmt.Println("Commits in sync branch not in main:")
-	logCmd := exec.CommandContext(ctx, "git", "log", "--oneline", currentBranch+".."+syncBranch)
+	logCmd := exec.CommandContext(ctx, "git", "log", "--oneline", currentBranch+".."+syncBranch) //nolint:gosec // branch names from git
 	logOutput, err := logCmd.CombinedOutput()
 	if err != nil {
 		return fmt.Errorf("failed to get commit log: %w\n%s", err, logOutput)
@@ -81,7 +81,7 @@ func showSyncStatus(ctx context.Context) error {
 	}

 	fmt.Println("\nCommits in main not in sync branch:")
-	logCmd = exec.CommandContext(ctx, "git", "log", "--oneline", syncBranch+".."+currentBranch)
+	logCmd = exec.CommandContext(ctx, "git", "log", "--oneline", syncBranch+".."+currentBranch) //nolint:gosec // branch names from git
 	logOutput, err = logCmd.CombinedOutput()
 	if err != nil {
 		return fmt.Errorf("failed to get commit log: %w\n%s", err, logOutput)
@@ -95,7 +95,7 @@ func showSyncStatus(ctx context.Context) error {

 	// Show file diff for .beads/issues.jsonl
 	fmt.Println("\nFile differences in .beads/issues.jsonl:")
-	diffCmd := exec.CommandContext(ctx, "git", "diff", currentBranch+"..."+syncBranch, "--", ".beads/issues.jsonl")
+	diffCmd := exec.CommandContext(ctx, "git", "diff", currentBranch+"..."+syncBranch, "--", ".beads/issues.jsonl") //nolint:gosec // branch names from git
 	diffOutput, err := diffCmd.CombinedOutput()
 	if err != nil {
 		// diff returns non-zero when there are differences, which is fine
@@ -130,7 +130,7 @@ func mergeSyncBranch(ctx context.Context, dryRun bool) error {
 	}

 	// Check if sync branch exists
-	checkCmd := exec.CommandContext(ctx, "git", "show-ref", "--verify", "--quiet", "refs/heads/"+syncBranch)
+	checkCmd := exec.CommandContext(ctx, "git", "show-ref", "--verify", "--quiet", "refs/heads/"+syncBranch) //nolint:gosec // syncBranch from config
 	if err := checkCmd.Run(); err != nil {
 		return fmt.Errorf("sync branch '%s' does not exist", syncBranch)
 	}
@@ -150,7 +150,7 @@ func mergeSyncBranch(ctx context.Context, dryRun bool) error {
 	if dryRun {
 		fmt.Println("→ [DRY RUN] Would merge sync branch")
 		// Show what would be merged
-		logCmd := exec.CommandContext(ctx, "git", "log", "--oneline", currentBranch+".."+syncBranch)
+		logCmd := exec.CommandContext(ctx, "git", "log", "--oneline", currentBranch+".."+syncBranch) //nolint:gosec // branch names from git
 		logOutput, _ := logCmd.CombinedOutput()
 		if len(strings.TrimSpace(string(logOutput))) > 0 {
 			fmt.Println("\nCommits that would be merged:")
@@ -162,7 +162,7 @@ func mergeSyncBranch(ctx context.Context, dryRun bool) error {
 	}

 	// Perform the merge
-	mergeCmd := exec.CommandContext(ctx, "git", "merge", syncBranch, "-m", fmt.Sprintf("Merge sync branch '%s'", syncBranch))
+	mergeCmd := exec.CommandContext(ctx, "git", "merge", syncBranch, "-m", fmt.Sprintf("Merge sync branch '%s'", syncBranch)) //nolint:gosec // syncBranch from config
 	mergeOutput, err := mergeCmd.CombinedOutput()
 	if err != nil {
 		return fmt.Errorf("merge failed: %w\n%s", err, mergeOutput)
@@ -228,13 +228,13 @@ func commitToExternalBeadsRepo(ctx context.Context, beadsDir, message string, pu
 		relBeadsDir = beadsDir // Fallback to absolute path
 	}

-	addCmd := exec.CommandContext(ctx, "git", "-C", repoRoot, "add", relBeadsDir)
+	addCmd := exec.CommandContext(ctx, "git", "-C", repoRoot, "add", relBeadsDir) //nolint:gosec // paths from trusted sources
 	if output, err := addCmd.CombinedOutput(); err != nil {
 		return false, fmt.Errorf("git add failed: %w\n%s", err, output)
 	}

 	// Check if there are staged changes
-	diffCmd := exec.CommandContext(ctx, "git", "-C", repoRoot, "diff", "--cached", "--quiet")
+	diffCmd := exec.CommandContext(ctx, "git", "-C", repoRoot, "diff", "--cached", "--quiet") //nolint:gosec // repoRoot from git
 	if diffCmd.Run() == nil {
 		return false, nil // No changes to commit
 	}
@@ -244,14 +244,14 @@ func commitToExternalBeadsRepo(ctx context.Context, beadsDir, message string, pu
 		message = fmt.Sprintf("bd sync: %s", time.Now().Format("2006-01-02 15:04:05"))
 	}
 	commitArgs := buildGitCommitArgs(repoRoot, message)
-	commitCmd := exec.CommandContext(ctx, "git", commitArgs...)
+	commitCmd := exec.CommandContext(ctx, "git", commitArgs...) //nolint:gosec // args from buildGitCommitArgs
 	if output, err := commitCmd.CombinedOutput(); err != nil {
 		return false, fmt.Errorf("git commit failed: %w\n%s", err, output)
 	}

 	// Push if requested
 	if push {
-		pushCmd := exec.CommandContext(ctx, "git", "-C", repoRoot, "push")
+		pushCmd := exec.CommandContext(ctx, "git", "-C", repoRoot, "push") //nolint:gosec // repoRoot from git
 		if pushOutput, err := runGitCmdWithTimeoutMsg(ctx, pushCmd, "git push", 5*time.Second); err != nil {
 			return true, fmt.Errorf("git push failed: %w\n%s", err, pushOutput)
 		}
@@ -270,13 +270,13 @@ func pullFromExternalBeadsRepo(ctx context.Context, beadsDir string) error {
 	}

 	// Check if remote exists
-	remoteCmd := exec.CommandContext(ctx, "git", "-C", repoRoot, "remote")
+	remoteCmd := exec.CommandContext(ctx, "git", "-C", repoRoot, "remote") //nolint:gosec // repoRoot from git
 	remoteOutput, err := remoteCmd.Output()
 	if err != nil || len(strings.TrimSpace(string(remoteOutput))) == 0 {
 		return nil // No remote, skip pull
 	}

-	pullCmd := exec.CommandContext(ctx, "git", "-C", repoRoot, "pull")
+	pullCmd := exec.CommandContext(ctx, "git", "-C", repoRoot, "pull") //nolint:gosec // repoRoot from git
 	if output, err := pullCmd.CombinedOutput(); err != nil {
 		return fmt.Errorf("git pull failed: %w\n%s", err, output)
 	}
--- a/cmd/bd/sync_check.go
+++ b/cmd/bd/sync_check.go
@@ -130,14 +130,14 @@ func checkForcedPush(ctx context.Context) *ForcedPushCheck {
 	}

 	// Check if sync branch exists locally
-	checkLocalCmd := exec.CommandContext(ctx, "git", "show-ref", "--verify", "--quiet", "refs/heads/"+syncBranch)
+	checkLocalCmd := exec.CommandContext(ctx, "git", "show-ref", "--verify", "--quiet", "refs/heads/"+syncBranch) //nolint:gosec // syncBranch from config
 	if checkLocalCmd.Run() != nil {
 		result.Message = fmt.Sprintf("Sync branch '%s' does not exist locally", syncBranch)
 		return result
 	}

 	// Get local ref
-	localRefCmd := exec.CommandContext(ctx, "git", "rev-parse", syncBranch)
+	localRefCmd := exec.CommandContext(ctx, "git", "rev-parse", syncBranch) //nolint:gosec // syncBranch from config
 	localRefOutput, err := localRefCmd.Output()
 	if err != nil {
 		result.Message = "Failed to get local sync branch ref"
@@ -153,7 +153,7 @@ func checkForcedPush(ctx context.Context) *ForcedPushCheck {
 	}

 	// Get remote ref
-	remoteRefCmd := exec.CommandContext(ctx, "git", "rev-parse", remote+"/"+syncBranch)
+	remoteRefCmd := exec.CommandContext(ctx, "git", "rev-parse", remote+"/"+syncBranch) //nolint:gosec // remote and syncBranch from config
 	remoteRefOutput, err := remoteRefCmd.Output()
 	if err != nil {
 		result.Message = fmt.Sprintf("Remote tracking branch '%s/%s' does not exist", remote, syncBranch)
@@ -169,14 +169,14 @@ func checkForcedPush(ctx context.Context) *ForcedPushCheck {
 	}

 	// Check if local is ahead of remote (normal case)
-	aheadCmd := exec.CommandContext(ctx, "git", "merge-base", "--is-ancestor", remoteRef, localRef)
+	aheadCmd := exec.CommandContext(ctx, "git", "merge-base", "--is-ancestor", remoteRef, localRef) //nolint:gosec // refs from git rev-parse
 	if aheadCmd.Run() == nil {
 		result.Message = "Local sync branch is ahead of remote (normal)"
 		return result
 	}

 	// Check if remote is ahead of local (behind, needs pull)
-	behindCmd := exec.CommandContext(ctx, "git", "merge-base", "--is-ancestor", localRef, remoteRef)
+	behindCmd := exec.CommandContext(ctx, "git", "merge-base", "--is-ancestor", localRef, remoteRef) //nolint:gosec // refs from git rev-parse
 	if behindCmd.Run() == nil {
 		result.Message = "Local sync branch is behind remote (needs pull)"
 		return result
--- a/cmd/bd/sync_import.go
+++ b/cmd/bd/sync_import.go
@@ -109,14 +109,14 @@ func doSyncFromMain(ctx context.Context, jsonlPath string, renameOnImport bool,

 	// Step 1: Fetch from main
 	fmt.Printf("→ Fetching from %s/%s...\n", remote, defaultBranch)
-	fetchCmd := exec.CommandContext(ctx, "git", "fetch", remote, defaultBranch)
+	fetchCmd := exec.CommandContext(ctx, "git", "fetch", remote, defaultBranch) //nolint:gosec // remote and defaultBranch from config
 	if output, err := fetchCmd.CombinedOutput(); err != nil {
 		return fmt.Errorf("git fetch %s %s failed: %w\n%s", remote, defaultBranch, err, output)
 	}

 	// Step 2: Checkout .beads/ directory from main
 	fmt.Printf("→ Checking out beads from %s/%s...\n", remote, defaultBranch)
-	checkoutCmd := exec.CommandContext(ctx, "git", "checkout", fmt.Sprintf("%s/%s", remote, defaultBranch), "--", ".beads/")
+	checkoutCmd := exec.CommandContext(ctx, "git", "checkout", fmt.Sprintf("%s/%s", remote, defaultBranch), "--", ".beads/") //nolint:gosec // remote and defaultBranch from config
 	if output, err := checkoutCmd.CombinedOutput(); err != nil {
 		return fmt.Errorf("git checkout .beads/ from %s/%s failed: %w\n%s", remote, defaultBranch, err, output)
 	}
--- a/internal/config/yaml_config.go
+++ b/internal/config/yaml_config.go
@@ -83,7 +83,7 @@ func SetYamlConfig(key, value string) error {
 	}

 	// Read existing config
-	content, err := os.ReadFile(configPath)
+	content, err := os.ReadFile(configPath) //nolint:gosec // configPath is from findProjectConfigYaml
 	if err != nil {
 		return fmt.Errorf("failed to read config.yaml: %w", err)
 	}
@@ -95,7 +95,7 @@ func SetYamlConfig(key, value string) error {
 	}

 	// Write back
-	if err := os.WriteFile(configPath, []byte(newContent), 0644); err != nil {
+	if err := os.WriteFile(configPath, []byte(newContent), 0600); err != nil { //nolint:gosec // configPath is validated
 		return fmt.Errorf("failed to write config.yaml: %w", err)
 	}

@@ -132,6 +132,8 @@ func findProjectConfigYaml() (string, error) {
 // updateYamlKey updates a key in yaml content, handling commented-out keys.
 // If the key exists (commented or not), it updates it in place.
 // If the key doesn't exist, it appends it at the end.
+//
+//nolint:unparam // error return kept for future validation
 func updateYamlKey(content, key, value string) (string, error) {
 	// Format the value appropriately
 	formattedValue := formatYamlValue(value)
--- a/internal/rpc/server_mutations_test.go
+++ b/internal/rpc/server_mutations_test.go
@@ -712,96 +712,6 @@ func TestHandleDelete_BatchEmitsMutations(t *testing.T) {
 	}
 }

-// TestHandleDelete_DryRun verifies that dry-run mode returns preview without actual deletion
-func TestHandleDelete_DryRun(t *testing.T) {
-	store := memory.New("/tmp/test.jsonl")
-	server := NewServer("/tmp/test.sock", store, "/tmp", "/tmp/test.db")
-
-	// Create test issues
-	issueIDs := make([]string, 2)
-	for i := 0; i < 2; i++ {
-		createArgs := CreateArgs{
-			Title:     "Issue for Dry Run " + string(rune('A'+i)),
-			IssueType: "task",
-			Priority:  2,
-		}
-		createJSON, _ := json.Marshal(createArgs)
-		createReq := &Request{
-			Operation: OpCreate,
-			Args:      createJSON,
-			Actor:     "test-user",
-		}
-
-		createResp := server.handleCreate(createReq)
-		if !createResp.Success {
-			t.Fatalf("failed to create test issue %d: %s", i, createResp.Error)
-		}
-
-		var createdIssue map[string]interface{}
-		if err := json.Unmarshal(createResp.Data, &createdIssue); err != nil {
-			t.Fatalf("failed to parse created issue %d: %v", i, err)
-		}
-		issueIDs[i] = createdIssue["id"].(string)
-	}
-
-	// Clear mutation buffer
-	_ = server.GetRecentMutations(time.Now().UnixMilli())
-
-	// Dry-run delete
-	deleteArgs := DeleteArgs{
-		IDs:    issueIDs,
-		DryRun: true,
-	}
-	deleteJSON, _ := json.Marshal(deleteArgs)
-	deleteReq := &Request{
-		Operation: OpDelete,
-		Args:      deleteJSON,
-		Actor:     "test-user",
-	}
-
-	deleteResp := server.handleDelete(deleteReq)
-	if !deleteResp.Success {
-		t.Fatalf("dry-run delete operation failed: %s", deleteResp.Error)
-	}
-
-	// Parse response
-	var result map[string]interface{}
-	if err := json.Unmarshal(deleteResp.Data, &result); err != nil {
-		t.Fatalf("failed to parse delete response: %v", err)
-	}
-
-	// Verify dry-run response structure
-	if dryRun, ok := result["dry_run"].(bool); !ok || !dryRun {
-		t.Errorf("expected dry_run=true in response, got %v", result["dry_run"])
-	}
-
-	if issueCount, ok := result["issue_count"].(float64); !ok || int(issueCount) != 2 {
-		t.Errorf("expected issue_count=2 in response, got %v", result["issue_count"])
-	}
-
-	// Verify no mutation events were emitted (dry-run doesn't delete)
-	mutations := server.GetRecentMutations(0)
-	for _, m := range mutations {
-		if m.Type == MutationDelete {
-			t.Errorf("unexpected delete mutation in dry-run mode: %s", m.IssueID)
-		}
-	}
-
-	// Verify issues still exist (not actually deleted)
-	for _, id := range issueIDs {
-		showArgs := ShowArgs{ID: id}
-		showJSON, _ := json.Marshal(showArgs)
-		showReq := &Request{
-			Operation: OpShow,
-			Args:      showJSON,
-		}
-		showResp := server.handleShow(showReq)
-		if !showResp.Success {
-			t.Errorf("issue %s should still exist after dry-run, but got error: %s", id, showResp.Error)
-		}
-	}
-}
-
 // TestHandleDelete_ErrorEmptyIDs verifies error when no issue IDs provided
 func TestHandleDelete_ErrorEmptyIDs(t *testing.T) {
 	store := memory.New("/tmp/test.jsonl")
@@ -883,93 +793,6 @@ func TestHandleDelete_ErrorIssueNotFound(t *testing.T) {
 	}
 }

-// TestHandleDelete_PartialSuccess verifies partial success when some issues exist and some don't
-func TestHandleDelete_PartialSuccess(t *testing.T) {
-	store := memory.New("/tmp/test.jsonl")
-	server := NewServer("/tmp/test.sock", store, "/tmp", "/tmp/test.db")
-
-	// Create one valid issue
-	createArgs := CreateArgs{
-		Title:     "Valid Issue for Partial Delete",
-		IssueType: "bug",
-		Priority:  1,
-	}
-	createJSON, _ := json.Marshal(createArgs)
-	createReq := &Request{
-		Operation: OpCreate,
-		Args:      createJSON,
-		Actor:     "test-user",
-	}
-
-	createResp := server.handleCreate(createReq)
-	if !createResp.Success {
-		t.Fatalf("failed to create test issue: %s", createResp.Error)
-	}
-
-	var createdIssue map[string]interface{}
-	if err := json.Unmarshal(createResp.Data, &createdIssue); err != nil {
-		t.Fatalf("failed to parse created issue: %v", err)
-	}
-	validID := createdIssue["id"].(string)
-
-	// Try to delete both valid and invalid issues
-	deleteArgs := DeleteArgs{
-		IDs:   []string{validID, "bd-nonexistent-xyz"},
-		Force: true,
-	}
-	deleteJSON, _ := json.Marshal(deleteArgs)
-	deleteReq := &Request{
-		Operation: OpDelete,
-		Args:      deleteJSON,
-		Actor:     "test-user",
-	}
-
-	deleteResp := server.handleDelete(deleteReq)
-	if !deleteResp.Success {
-		t.Fatalf("partial delete should succeed with partial_success flag: %s", deleteResp.Error)
-	}
-
-	// Parse response
-	var result map[string]interface{}
-	if err := json.Unmarshal(deleteResp.Data, &result); err != nil {
-		t.Fatalf("failed to parse response: %v", err)
-	}
-
-	// Verify partial success
-	if deletedCount, ok := result["deleted_count"].(float64); !ok || int(deletedCount) != 1 {
-		t.Errorf("expected deleted_count=1, got %v", result["deleted_count"])
-	}
-
-	if totalCount, ok := result["total_count"].(float64); !ok || int(totalCount) != 2 {
-		t.Errorf("expected total_count=2, got %v", result["total_count"])
-	}
-
-	if partialSuccess, ok := result["partial_success"].(bool); !ok || !partialSuccess {
-		t.Errorf("expected partial_success=true, got %v", result["partial_success"])
-	}
-
-	// Verify errors array contains the not found error
-	if errors, ok := result["errors"].([]interface{}); ok {
-		if len(errors) != 1 {
-			t.Errorf("expected 1 error, got %d", len(errors))
-		}
-	} else {
-		t.Error("expected errors array in response")
-	}
-
-	// Verify the valid issue was actually deleted
-	showArgs := ShowArgs{ID: validID}
-	showJSON, _ := json.Marshal(showArgs)
-	showReq := &Request{
-		Operation: OpShow,
-		Args:      showJSON,
-	}
-	showResp := server.handleShow(showReq)
-	if showResp.Success {
-		t.Error("deleted issue should not be found")
-	}
-}
-
 // TestHandleDelete_ErrorCannotDeleteTemplate verifies that templates cannot be deleted
 func TestHandleDelete_ErrorCannotDeleteTemplate(t *testing.T) {
 	store := memory.New("/tmp/test.jsonl")