From 65214b7693d0f109d7da5d713c431c58e0520775 Mon Sep 17 00:00:00 2001 From: Peter Chanthamynavong Date: Thu, 8 Jan 2026 14:36:07 -0800 Subject: [PATCH] fix(rpc): return JSON data for dep add/remove in daemon mode (#961) * test(rpc): add failing tests for dep add/remove JSON output (GH#952) - Add TestDepAdd_JSONOutput: verify Response.Data contains JSON - Add TestDepRemove_JSONOutput: verify Response.Data contains JSON Tests prove bug exists: handlers return Response{Success: true} without populating Data field. * fix(rpc): return JSON data for dep add/remove in daemon mode (GH#952) Problem: - bd dep add/remove --json returned empty output in daemon mode - handleDepAdd() and handleSimpleStoreOp() didn't populate Response.Data Solution: - handleDepAdd: add Data field with JSON result - handleSimpleStoreOp: add optional responseData callback parameter - handleDepRemove: provide response data callback - Label handlers: pass nil (maintain current behavior) --- internal/rpc/server_labels_deps_comments.go | 36 +++- .../rpc/server_labels_deps_comments_test.go | 172 ++++++++++++++++++ 2 files changed, 201 insertions(+), 7 deletions(-) create mode 100644 internal/rpc/server_labels_deps_comments_test.go diff --git a/internal/rpc/server_labels_deps_comments.go b/internal/rpc/server_labels_deps_comments.go index f9e65142..6b203ff6 100644 --- a/internal/rpc/server_labels_deps_comments.go +++ b/internal/rpc/server_labels_deps_comments.go @@ -80,12 +80,20 @@ func (s *Server) handleDepAdd(req *Request) Response { title, assignee := s.lookupIssueMeta(ctx, depArgs.FromID) s.emitMutation(MutationUpdate, depArgs.FromID, title, assignee) - return Response{Success: true} + result := map[string]interface{}{ + "status": "added", + "issue_id": depArgs.FromID, + "depends_on_id": depArgs.ToID, + "type": depArgs.DepType, + } + data, _ := json.Marshal(result) + return Response{Success: true, Data: data} } // Generic handler for simple store operations with standard error handling func (s *Server) handleSimpleStoreOp(req *Request, argsPtr interface{}, argDesc string, - opFunc func(context.Context, storage.Storage, string) error, issueID string) Response { + opFunc func(context.Context, storage.Storage, string) error, issueID string, + responseData func() map[string]interface{}) Response { if err := json.Unmarshal(req.Args, argsPtr); err != nil { return Response{ Success: false, @@ -113,28 +121,42 @@ func (s *Server) handleSimpleStoreOp(req *Request, argsPtr interface{}, argDesc title, assignee := s.lookupIssueMeta(ctx, issueID) s.emitMutation(MutationUpdate, issueID, title, assignee) + if responseData != nil { + data, _ := json.Marshal(responseData()) + return Response{Success: true, Data: data} + } return Response{Success: true} } func (s *Server) handleDepRemove(req *Request) Response { var depArgs DepRemoveArgs - return s.handleSimpleStoreOp(req, &depArgs, "dep remove", func(ctx context.Context, store storage.Storage, actor string) error { - return store.RemoveDependency(ctx, depArgs.FromID, depArgs.ToID, actor) - }, depArgs.FromID) + return s.handleSimpleStoreOp(req, &depArgs, "dep remove", + func(ctx context.Context, store storage.Storage, actor string) error { + return store.RemoveDependency(ctx, depArgs.FromID, depArgs.ToID, actor) + }, + depArgs.FromID, + func() map[string]interface{} { + return map[string]interface{}{ + "status": "removed", + "issue_id": depArgs.FromID, + "depends_on_id": depArgs.ToID, + } + }, + ) } func (s *Server) handleLabelAdd(req *Request) Response { var labelArgs LabelAddArgs return s.handleSimpleStoreOp(req, &labelArgs, "label add", func(ctx context.Context, store storage.Storage, actor string) error { return store.AddLabel(ctx, labelArgs.ID, labelArgs.Label, actor) - }, labelArgs.ID) + }, labelArgs.ID, nil) } func (s *Server) handleLabelRemove(req *Request) Response { var labelArgs LabelRemoveArgs return s.handleSimpleStoreOp(req, &labelArgs, "label remove", func(ctx context.Context, store storage.Storage, actor string) error { return store.RemoveLabel(ctx, labelArgs.ID, labelArgs.Label, actor) - }, labelArgs.ID) + }, labelArgs.ID, nil) } func (s *Server) handleCommentList(req *Request) Response { diff --git a/internal/rpc/server_labels_deps_comments_test.go b/internal/rpc/server_labels_deps_comments_test.go new file mode 100644 index 00000000..4aa28915 --- /dev/null +++ b/internal/rpc/server_labels_deps_comments_test.go @@ -0,0 +1,172 @@ +package rpc + +import ( + "encoding/json" + "testing" +) + +// TestDepAdd_JSONOutput verifies that handleDepAdd returns JSON data in Response.Data. +// This test is expected to FAIL until the bug is fixed (GH#952 Issue 2). +func TestDepAdd_JSONOutput(t *testing.T) { + _, client, store, cleanup := setupTestServerWithStore(t) + defer cleanup() + + // Create two test issues for the dependency relationship + createArgs1 := &CreateArgs{ + Title: "Issue that depends on another", + IssueType: "task", + Priority: 2, + } + resp1, err := client.Create(createArgs1) + if err != nil { + t.Fatalf("Failed to create first issue: %v", err) + } + var issue1 struct{ ID string } + if err := json.Unmarshal(resp1.Data, &issue1); err != nil { + t.Fatalf("Failed to unmarshal first issue: %v", err) + } + + createArgs2 := &CreateArgs{ + Title: "Issue being depended upon", + IssueType: "task", + Priority: 2, + } + resp2, err := client.Create(createArgs2) + if err != nil { + t.Fatalf("Failed to create second issue: %v", err) + } + var issue2 struct{ ID string } + if err := json.Unmarshal(resp2.Data, &issue2); err != nil { + t.Fatalf("Failed to unmarshal second issue: %v", err) + } + + // Add dependency: issue1 depends on issue2 + depArgs := &DepAddArgs{ + FromID: issue1.ID, + ToID: issue2.ID, + DepType: "blocks", + } + resp, err := client.AddDependency(depArgs) + if err != nil { + t.Fatalf("AddDependency failed: %v", err) + } + + // BUG: Response.Data is nil when it should contain JSON + if resp.Data == nil { + t.Errorf("resp.Data is nil; expected JSON output with {status, issue_id, depends_on_id, type}") + } + + // Verify JSON structure matches expected format + if resp.Data != nil { + var result struct { + Status string `json:"status"` + IssueID string `json:"issue_id"` + DependsOnID string `json:"depends_on_id"` + Type string `json:"type"` + } + if err := json.Unmarshal(resp.Data, &result); err != nil { + t.Errorf("Failed to unmarshal response data: %v", err) + } + if result.Status != "added" { + t.Errorf("Expected status='added', got %q", result.Status) + } + if result.IssueID != issue1.ID { + t.Errorf("Expected issue_id=%q, got %q", issue1.ID, result.IssueID) + } + if result.DependsOnID != issue2.ID { + t.Errorf("Expected depends_on_id=%q, got %q", issue2.ID, result.DependsOnID) + } + if result.Type != "blocks" { + t.Errorf("Expected type='blocks', got %q", result.Type) + } + } + + // Silence unused variable warning + _ = store +} + +// TestDepRemove_JSONOutput verifies that handleDepRemove returns JSON data in Response.Data. +// This test is expected to FAIL until the bug is fixed (GH#952 Issue 2). +func TestDepRemove_JSONOutput(t *testing.T) { + _, client, store, cleanup := setupTestServerWithStore(t) + defer cleanup() + + // Create two test issues + createArgs1 := &CreateArgs{ + Title: "Issue with dependency to remove", + IssueType: "task", + Priority: 2, + } + resp1, err := client.Create(createArgs1) + if err != nil { + t.Fatalf("Failed to create first issue: %v", err) + } + var issue1 struct{ ID string } + if err := json.Unmarshal(resp1.Data, &issue1); err != nil { + t.Fatalf("Failed to unmarshal first issue: %v", err) + } + + createArgs2 := &CreateArgs{ + Title: "Dependency target issue", + IssueType: "task", + Priority: 2, + } + resp2, err := client.Create(createArgs2) + if err != nil { + t.Fatalf("Failed to create second issue: %v", err) + } + var issue2 struct{ ID string } + if err := json.Unmarshal(resp2.Data, &issue2); err != nil { + t.Fatalf("Failed to unmarshal second issue: %v", err) + } + + // First add a dependency so we can remove it + addArgs := &DepAddArgs{ + FromID: issue1.ID, + ToID: issue2.ID, + DepType: "blocks", + } + _, err = client.AddDependency(addArgs) + if err != nil { + t.Fatalf("AddDependency (setup) failed: %v", err) + } + + // Now remove the dependency + removeArgs := &DepRemoveArgs{ + FromID: issue1.ID, + ToID: issue2.ID, + } + resp, err := client.RemoveDependency(removeArgs) + if err != nil { + t.Fatalf("RemoveDependency failed: %v", err) + } + + // BUG: Response.Data is nil when it should contain JSON + if resp.Data == nil { + t.Errorf("resp.Data is nil; expected JSON output with {status, issue_id, depends_on_id}") + } + + // Verify JSON structure matches expected format + if resp.Data != nil { + var result struct { + Status string `json:"status"` + IssueID string `json:"issue_id"` + DependsOnID string `json:"depends_on_id"` + } + if err := json.Unmarshal(resp.Data, &result); err != nil { + t.Errorf("Failed to unmarshal response data: %v", err) + } + if result.Status != "removed" { + t.Errorf("Expected status='removed', got %q", result.Status) + } + if result.IssueID != issue1.ID { + t.Errorf("Expected issue_id=%q, got %q", issue1.ID, result.IssueID) + } + if result.DependsOnID != issue2.ID { + t.Errorf("Expected depends_on_id=%q, got %q", issue2.ID, result.DependsOnID) + } + } + + // Silence unused variable warning + _ = store +}