Merge pull request #1071 from pentop/fix/comments-short-id

fix: resolve short IDs in comments add/list daemon mode (#1070)
2026-01-13 11:41:42 -08:00
parent a2bf15ee48 66007ac3ea
commit d28d1f0788
3 changed files with 297 additions and 0 deletions
--- a/cmd/bd/cli_fast_test.go
+++ b/cmd/bd/cli_fast_test.go
@@ -972,4 +972,128 @@ func TestCLI_CreateDryRun(t *testing.T) {
 	})
 }

+// TestCLI_CommentsAddShortID tests that 'comments add' accepts short IDs (issue #1070)
+// Most bd commands accept short IDs (e.g., "5wbm") but comments add previously required
+// full IDs (e.g., "mike.vibe-coding-5wbm"). This test ensures short IDs work.
+//
+// Note: This test runs with --no-daemon (direct mode) where short IDs already work
+// because the code calls utils.ResolvePartialID(). The actual bug (GitHub #1070) is
+// in daemon mode where the ID isn't resolved before being sent to the RPC server.
+// The fix should add daemonClient.ResolveID() before daemonClient.AddComment(),
+// following the pattern in update.go and label.go.
+func TestCLI_CommentsAddShortID(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping slow CLI test in short mode")
+	}
+
+	t.Run("ShortIDWithCommentsAdd", func(t *testing.T) {
+		tmpDir := setupCLITestDB(t)
+
+		// Create an issue and get its full ID
+		out := runBDInProcess(t, tmpDir, "create", "Issue for comment test", "-p", "1", "--json")
+
+		jsonStart := strings.Index(out, "{")
+		if jsonStart < 0 {
+			t.Fatalf("No JSON found in output: %s", out)
+		}
+		jsonOut := out[jsonStart:]
+
+		var issue map[string]interface{}
+		if err := json.Unmarshal([]byte(jsonOut), &issue); err != nil {
+			t.Fatalf("Failed to parse JSON: %v\nOutput: %s", err, jsonOut)
+		}
+
+		fullID := issue["id"].(string)
+		t.Logf("Created issue with full ID: %s", fullID)
+
+		// Extract short ID (the part after the last hyphen in prefix-hash format)
+		// For IDs like "test-abc123", the short ID is "abc123"
+		parts := strings.Split(fullID, "-")
+		if len(parts) < 2 {
+			t.Fatalf("Unexpected ID format: %s", fullID)
+		}
+		shortID := parts[len(parts)-1]
+		t.Logf("Using short ID: %s", shortID)
+
+		// Add a comment using the SHORT ID (not full ID)
+		stdout, stderr, err := runBDInProcessAllowError(t, tmpDir, "comments", "add", shortID, "Test comment with short ID")
+		if err != nil {
+			t.Fatalf("comments add failed: %v\nstdout: %s\nstderr: %s", err, stdout, stderr)
+		}
+
+		if !strings.Contains(stdout, "Comment added") {
+			t.Errorf("Expected 'Comment added' in output, got: %s", stdout)
+		}
+
+		// Verify the comment was actually added by listing comments (use full ID for list)
+		stdout, stderr, err = runBDInProcessAllowError(t, tmpDir, "comments", fullID)
+		if err != nil {
+			t.Fatalf("comments list failed: %v\nstdout: %s\nstderr: %s", err, stdout, stderr)
+		}
+
+		if !strings.Contains(stdout, "Test comment with short ID") {
+			t.Errorf("Expected comment text in list output, got: %s", stdout)
+		}
+	})
+
+	t.Run("PartialIDWithCommentsAdd", func(t *testing.T) {
+		tmpDir := setupCLITestDB(t)
+
+		// Create an issue
+		out := runBDInProcess(t, tmpDir, "create", "Issue for partial ID test", "-p", "1", "--json")
+
+		jsonStart := strings.Index(out, "{")
+		jsonOut := out[jsonStart:]
+
+		var issue map[string]interface{}
+		json.Unmarshal([]byte(jsonOut), &issue)
+		fullID := issue["id"].(string)
+
+		// Extract short ID and use only first 4 characters (partial match)
+		parts := strings.Split(fullID, "-")
+		shortID := parts[len(parts)-1]
+		if len(shortID) > 4 {
+			shortID = shortID[:4] // Use only first 4 chars for partial match
+		}
+		t.Logf("Full ID: %s, Partial ID: %s", fullID, shortID)
+
+		// Add comment using partial ID
+		stdout, stderr, err := runBDInProcessAllowError(t, tmpDir, "comments", "add", shortID, "Comment via partial ID")
+		if err != nil {
+			t.Fatalf("comments add with partial ID failed: %v\nstdout: %s\nstderr: %s", err, stdout, stderr)
+		}
+
+		if !strings.Contains(stdout, "Comment added") {
+			t.Errorf("Expected 'Comment added' in output, got: %s", stdout)
+		}
+	})
+
+	t.Run("CommentAliasWithShortID", func(t *testing.T) {
+		tmpDir := setupCLITestDB(t)
+
+		// Create an issue
+		out := runBDInProcess(t, tmpDir, "create", "Issue for alias test", "-p", "1", "--json")
+
+		jsonStart := strings.Index(out, "{")
+		jsonOut := out[jsonStart:]
+
+		var issue map[string]interface{}
+		json.Unmarshal([]byte(jsonOut), &issue)
+		fullID := issue["id"].(string)
+
+		// Extract short ID
+		parts := strings.Split(fullID, "-")
+		shortID := parts[len(parts)-1]
+
+		// Use the 'comment' alias (deprecated but should still work)
+		stdout, stderr, err := runBDInProcessAllowError(t, tmpDir, "comment", shortID, "Comment via alias with short ID")
+		if err != nil {
+			t.Fatalf("comment alias failed: %v\nstdout: %s\nstderr: %s", err, stdout, stderr)
+		}
+
+		if !strings.Contains(stdout, "Comment added") {
+			t.Errorf("Expected 'Comment added' in output, got: %s", stdout)
+		}
+	})
+}

--- a/cmd/bd/comments.go
+++ b/cmd/bd/comments.go
@@ -38,6 +38,18 @@ Examples:
 		comments := make([]*types.Comment, 0)
 		usedDaemon := false
 		if daemonClient != nil {
+			// Resolve short/partial ID to full ID before sending to daemon (#1070)
+			resolveArgs := &rpc.ResolveIDArgs{ID: issueID}
+			resolveResp, err := daemonClient.ResolveID(resolveArgs)
+			if err != nil {
+				FatalErrorRespectJSON("resolving ID %s: %v", issueID, err)
+			}
+			var resolvedID string
+			if err := json.Unmarshal(resolveResp.Data, &resolvedID); err != nil {
+				FatalErrorRespectJSON("unmarshaling resolved ID: %v", err)
+			}
+			issueID = resolvedID
+
 			resp, err := daemonClient.ListComments(&rpc.CommentListArgs{ID: issueID})
 			if err != nil {
 				if isUnknownOperationError(err) {
@@ -145,6 +157,18 @@ Examples:

 		var comment *types.Comment
 		if daemonClient != nil {
+			// Resolve short/partial ID to full ID before sending to daemon (#1070)
+			resolveArgs := &rpc.ResolveIDArgs{ID: issueID}
+			resolveResp, err := daemonClient.ResolveID(resolveArgs)
+			if err != nil {
+				FatalErrorRespectJSON("resolving ID %s: %v", issueID, err)
+			}
+			var resolvedID string
+			if err := json.Unmarshal(resolveResp.Data, &resolvedID); err != nil {
+				FatalErrorRespectJSON("unmarshaling resolved ID: %v", err)
+			}
+			issueID = resolvedID
+
 			resp, err := daemonClient.AddComment(&rpc.CommentAddArgs{
 				ID:     issueID,
 				Author: author,
--- a/internal/rpc/comments_test.go
+++ b/internal/rpc/comments_test.go
@@ -69,3 +69,152 @@ func TestCommentOperationsViaRPC(t *testing.T) {
 		t.Fatalf("expected comment text 'first comment', got %q", comments[0].Text)
 	}
 }
+
+// TestCommentAddWithResolvedID verifies that the RPC layer works correctly
+// when the CLI resolves short IDs before sending to the daemon (issue #1070).
+//
+// Note: The RPC server expects full IDs. Short ID resolution happens in the CLI
+// (cmd/bd/comments.go) before calling the daemon, following the pattern used by
+// update.go, label.go, and other commands. This test simulates that workflow.
+func TestCommentAddWithResolvedID(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping slow RPC test in short mode")
+	}
+	_, client, cleanup := setupTestServer(t)
+	defer cleanup()
+
+	// Create an issue
+	createResp, err := client.Create(&CreateArgs{
+		Title:     "Resolved ID comment test",
+		IssueType: "task",
+		Priority:  2,
+	})
+	if err != nil {
+		t.Fatalf("create issue failed: %v", err)
+	}
+
+	var created types.Issue
+	if err := json.Unmarshal(createResp.Data, &created); err != nil {
+		t.Fatalf("failed to decode create response: %v", err)
+	}
+
+	fullID := created.ID
+	t.Logf("Created issue with full ID: %s", fullID)
+
+	// Extract the short ID (hash portion after the prefix)
+	shortID := fullID
+	for i := len(fullID) - 1; i >= 0; i-- {
+		if fullID[i] == '-' {
+			shortID = fullID[i+1:]
+			break
+		}
+	}
+	t.Logf("Short ID: %s", shortID)
+
+	// Simulate CLI behavior: resolve short ID first, then call RPC with full ID
+	// In the real CLI, this is done via daemonClient.ResolveID()
+	resolveResp, err := client.ResolveID(&ResolveIDArgs{ID: shortID})
+	if err != nil {
+		t.Fatalf("resolve ID failed: %v", err)
+	}
+	var resolvedID string
+	if err := json.Unmarshal(resolveResp.Data, &resolvedID); err != nil {
+		t.Fatalf("failed to decode resolved ID: %v", err)
+	}
+	t.Logf("Resolved ID: %s", resolvedID)
+
+	if resolvedID != fullID {
+		t.Fatalf("expected resolved ID %q, got %q", fullID, resolvedID)
+	}
+
+	// Now add comment using the RESOLVED (full) ID
+	addResp, err := client.AddComment(&CommentAddArgs{
+		ID:     resolvedID,
+		Author: "tester",
+		Text:   "comment via resolved ID",
+	})
+	if err != nil {
+		t.Fatalf("add comment failed: %v", err)
+	}
+
+	var added types.Comment
+	if err := json.Unmarshal(addResp.Data, &added); err != nil {
+		t.Fatalf("failed to decode add comment response: %v", err)
+	}
+
+	if added.Text != "comment via resolved ID" {
+		t.Errorf("expected comment text 'comment via resolved ID', got %q", added.Text)
+	}
+}
+
+// TestCommentListWithResolvedID verifies that ListComments works when the CLI
+// resolves short IDs before sending to the daemon (issue #1070).
+func TestCommentListWithResolvedID(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping slow RPC test in short mode")
+	}
+	_, client, cleanup := setupTestServer(t)
+	defer cleanup()
+
+	// Create an issue and add a comment
+	createResp, err := client.Create(&CreateArgs{
+		Title:     "Resolved ID list test",
+		IssueType: "task",
+		Priority:  2,
+	})
+	if err != nil {
+		t.Fatalf("create issue failed: %v", err)
+	}
+
+	var created types.Issue
+	if err := json.Unmarshal(createResp.Data, &created); err != nil {
+		t.Fatalf("failed to decode create response: %v", err)
+	}
+
+	fullID := created.ID
+
+	// Add a comment using full ID
+	_, err = client.AddComment(&CommentAddArgs{
+		ID:     fullID,
+		Author: "tester",
+		Text:   "test comment for list",
+	})
+	if err != nil {
+		t.Fatalf("add comment failed: %v", err)
+	}
+
+	// Extract short ID
+	shortID := fullID
+	for i := len(fullID) - 1; i >= 0; i-- {
+		if fullID[i] == '-' {
+			shortID = fullID[i+1:]
+			break
+		}
+	}
+	t.Logf("Full ID: %s, Short ID: %s", fullID, shortID)
+
+	// Simulate CLI behavior: resolve short ID first
+	resolveResp, err := client.ResolveID(&ResolveIDArgs{ID: shortID})
+	if err != nil {
+		t.Fatalf("resolve ID failed: %v", err)
+	}
+	var resolvedID string
+	if err := json.Unmarshal(resolveResp.Data, &resolvedID); err != nil {
+		t.Fatalf("failed to decode resolved ID: %v", err)
+	}
+
+	// List comments using the RESOLVED (full) ID
+	listResp, err := client.ListComments(&CommentListArgs{ID: resolvedID})
+	if err != nil {
+		t.Fatalf("list comments failed: %v", err)
+	}
+
+	var comments []*types.Comment
+	if err := json.Unmarshal(listResp.Data, &comments); err != nil {
+		t.Fatalf("failed to decode comment list: %v", err)
+	}
+
+	if len(comments) != 1 {
+		t.Fatalf("expected 1 comment, got %d", len(comments))
+	}
+}