Skip to content

Commit 007b354

Browse files
JAORMXclaude
JAORMX
and
claude
committed
Address PR feedback on capability discovery implementation
- Fix test key generation to use fmt.Sprintf instead of rune arithmetic - Discover all workloads regardless of status and map to health levels - Return empty list instead of error when no backends found - Add workload_status to backend metadata for observability This addresses reviewer feedback from PR #2354, ensuring the discoverer provides complete information about all backends in a group rather than filtering at discovery time. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 76dabc2 commit 007b354

File tree

3 files changed

+109
-41
lines changed

3 files changed

+109
-41
lines changed

‎pkg/vmcp/aggregator/cli_discoverer.go‎

Lines changed: 33 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ func NewCLIBackendDiscoverer(workloadsManager workloads.Manager, groupsManager g
2828
}
2929

3030
// Discover finds all backend workloads in the specified group.
31-
// Returns only healthy/running backends.
31+
// Returns all accessible backends with their health status marked based on workload status.
3232
// The groupRef is the group name (e.g., "engineering-team").
3333
func (d *cliBackendDiscoverer) Discover(ctx context.Context, groupRef string) ([]vmcp.Backend, error) {
3434
logger.Infof("Discovering backends in group %s", groupRef)
@@ -49,13 +49,13 @@ func (d *cliBackendDiscoverer) Discover(ctx context.Context, groupRef string) ([
4949
}
5050

5151
if len(workloadNames) == 0 {
52-
logger.Warnf("No workloads found in group %s", groupRef)
53-
return nil, ErrNoBackendsFound
52+
logger.Infof("No workloads found in group %s", groupRef)
53+
return []vmcp.Backend{}, nil
5454
}
5555

56-
logger.Debugf("Found %d workloads in group %s, filtering for healthy backends", len(workloadNames), groupRef)
56+
logger.Debugf("Found %d workloads in group %s, discovering backends", len(workloadNames), groupRef)
5757

58-
// Query each workload and filter for healthy ones
58+
// Query each workload and convert to backend
5959
var backends []vmcp.Backend
6060
for _, name := range workloadNames {
6161
workload, err := d.workloadsManager.GetWorkload(ctx, name)
@@ -64,28 +64,26 @@ func (d *cliBackendDiscoverer) Discover(ctx context.Context, groupRef string) ([
6464
continue
6565
}
6666

67-
// Only include running workloads
68-
if workload.Status != rt.WorkloadStatusRunning {
69-
logger.Debugf("Skipping workload %s with status %s", name, workload.Status)
70-
continue
71-
}
72-
7367
// Skip workloads without a URL (not accessible)
7468
if workload.URL == "" {
7569
logger.Debugf("Skipping workload %s without URL", name)
7670
continue
7771
}
7872

73+
// Map workload status to backend health status
74+
healthStatus := mapWorkloadStatusToHealth(workload.Status)
75+
7976
// Convert core.Workload to vmcp.Backend
8077
backend := vmcp.Backend{
8178
ID: name,
8279
Name: name,
8380
BaseURL: workload.URL,
8481
TransportType: workload.TransportType.String(),
85-
HealthStatus: vmcp.BackendHealthy,
82+
HealthStatus: healthStatus,
8683
Metadata: map[string]string{
87-
"group": groupRef,
88-
"tool_type": workload.ToolType,
84+
"group": groupRef,
85+
"tool_type": workload.ToolType,
86+
"workload_status": string(workload.Status),
8987
},
9088
}
9189

@@ -95,14 +93,31 @@ func (d *cliBackendDiscoverer) Discover(ctx context.Context, groupRef string) ([
9593
}
9694

9795
backends = append(backends, backend)
98-
logger.Debugf("Discovered backend %s: %s (%s)", backend.ID, backend.BaseURL, backend.TransportType)
96+
logger.Debugf("Discovered backend %s: %s (%s) with health status %s",
97+
backend.ID, backend.BaseURL, backend.TransportType, backend.HealthStatus)
9998
}
10099

101100
if len(backends) == 0 {
102-
logger.Warnf("No healthy backends found in group %s", groupRef)
103-
return nil, ErrNoBackendsFound
101+
logger.Infof("No accessible backends found in group %s (all workloads lack URLs)", groupRef)
102+
return []vmcp.Backend{}, nil
104103
}
105104

106-
logger.Infof("Discovered %d healthy backends in group %s", len(backends), groupRef)
105+
logger.Infof("Discovered %d backends in group %s", len(backends), groupRef)
107106
return backends, nil
108107
}
108+
109+
// mapWorkloadStatusToHealth converts a workload status to a backend health status.
110+
func mapWorkloadStatusToHealth(status rt.WorkloadStatus) vmcp.BackendHealthStatus {
111+
switch status {
112+
case rt.WorkloadStatusRunning:
113+
return vmcp.BackendHealthy
114+
case rt.WorkloadStatusUnhealthy:
115+
return vmcp.BackendUnhealthy
116+
case rt.WorkloadStatusStopped, rt.WorkloadStatusError, rt.WorkloadStatusStopping, rt.WorkloadStatusRemoving:
117+
return vmcp.BackendUnhealthy
118+
case rt.WorkloadStatusStarting, rt.WorkloadStatusUnknown:
119+
return vmcp.BackendUnknown
120+
default:
121+
return vmcp.BackendUnknown
122+
}
123+
}

‎pkg/vmcp/aggregator/cli_discoverer_test.go‎

Lines changed: 71 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ func TestCLIBackendDiscoverer_Discover(t *testing.T) {
9797
assert.Equal(t, "sse", backends[1].TransportType)
9898
})
9999

100-
t.Run("filters out stopped workloads", func(t *testing.T) {
100+
t.Run("discovers workloads with different statuses", func(t *testing.T) {
101101
t.Parallel()
102102
ctrl := gomock.NewController(t)
103103
defer ctrl.Finish()
@@ -143,8 +143,12 @@ func TestCLIBackendDiscoverer_Discover(t *testing.T) {
143143
backends, err := discoverer.Discover(context.Background(), groupName)
144144

145145
require.NoError(t, err)
146-
assert.Len(t, backends, 1)
146+
assert.Len(t, backends, 2)
147147
assert.Equal(t, "running-workload", backends[0].ID)
148+
assert.Equal(t, vmcp.BackendHealthy, backends[0].HealthStatus)
149+
assert.Equal(t, "stopped-workload", backends[1].ID)
150+
assert.Equal(t, vmcp.BackendUnhealthy, backends[1].HealthStatus)
151+
assert.Equal(t, "stopped", backends[1].Metadata["workload_status"])
148152
})
149153

150154
t.Run("filters out workloads without URL", func(t *testing.T) {
@@ -197,6 +201,53 @@ func TestCLIBackendDiscoverer_Discover(t *testing.T) {
197201
assert.Equal(t, "workload1", backends[0].ID)
198202
})
199203

204+
t.Run("returns empty list when all workloads lack URLs", func(t *testing.T) {
205+
t.Parallel()
206+
ctrl := gomock.NewController(t)
207+
defer ctrl.Finish()
208+
209+
mockWorkloads := workloadmocks.NewMockManager(ctrl)
210+
mockGroups := mocks.NewMockManager(ctrl)
211+
212+
groupName := testGroupName
213+
214+
mockGroups.EXPECT().
215+
Exists(gomock.Any(), groupName).
216+
Return(true, nil)
217+
218+
mockWorkloads.EXPECT().
219+
ListWorkloadsInGroup(gomock.Any(), groupName).
220+
Return([]string{"workload1", "workload2"}, nil)
221+
222+
workload1 := core.Workload{
223+
Name: "workload1",
224+
Status: runtime.WorkloadStatusRunning,
225+
URL: "", // No URL
226+
Group: groupName,
227+
}
228+
229+
workload2 := core.Workload{
230+
Name: "workload2",
231+
Status: runtime.WorkloadStatusStopped,
232+
URL: "", // No URL
233+
Group: groupName,
234+
}
235+
236+
mockWorkloads.EXPECT().
237+
GetWorkload(gomock.Any(), "workload1").
238+
Return(workload1, nil)
239+
240+
mockWorkloads.EXPECT().
241+
GetWorkload(gomock.Any(), "workload2").
242+
Return(workload2, nil)
243+
244+
discoverer := NewCLIBackendDiscoverer(mockWorkloads, mockGroups)
245+
backends, err := discoverer.Discover(context.Background(), groupName)
246+
247+
require.NoError(t, err)
248+
assert.Empty(t, backends)
249+
})
250+
200251
t.Run("returns error when group does not exist", func(t *testing.T) {
201252
t.Parallel()
202253
ctrl := gomock.NewController(t)
@@ -243,7 +294,7 @@ func TestCLIBackendDiscoverer_Discover(t *testing.T) {
243294
assert.Contains(t, err.Error(), "failed to check if group exists")
244295
})
245296

246-
t.Run("returns ErrNoBackendsFound when group is empty", func(t *testing.T) {
297+
t.Run("returns empty list when group is empty", func(t *testing.T) {
247298
t.Parallel()
248299
ctrl := gomock.NewController(t)
249300
defer ctrl.Finish()
@@ -264,12 +315,11 @@ func TestCLIBackendDiscoverer_Discover(t *testing.T) {
264315
discoverer := NewCLIBackendDiscoverer(mockWorkloads, mockGroups)
265316
backends, err := discoverer.Discover(context.Background(), groupName)
266317

267-
require.Error(t, err)
268-
assert.ErrorIs(t, err, ErrNoBackendsFound)
269-
assert.Nil(t, backends)
318+
require.NoError(t, err)
319+
assert.Empty(t, backends)
270320
})
271321

272-
t.Run("returns ErrNoBackendsFound when all workloads are unhealthy", func(t *testing.T) {
322+
t.Run("discovers all workloads regardless of health status", func(t *testing.T) {
273323
t.Parallel()
274324
ctrl := gomock.NewController(t)
275325
defer ctrl.Finish()
@@ -288,17 +338,19 @@ func TestCLIBackendDiscoverer_Discover(t *testing.T) {
288338
Return([]string{"stopped1", "error1"}, nil)
289339

290340
stoppedWorkload := core.Workload{
291-
Name: "stopped1",
292-
Status: runtime.WorkloadStatusStopped,
293-
URL: "http://localhost:8080/mcp",
294-
Group: groupName,
341+
Name: "stopped1",
342+
Status: runtime.WorkloadStatusStopped,
343+
URL: "http://localhost:8080/mcp",
344+
TransportType: types.TransportTypeStreamableHTTP,
345+
Group: groupName,
295346
}
296347

297348
errorWorkload := core.Workload{
298-
Name: "error1",
299-
Status: runtime.WorkloadStatusError,
300-
URL: "http://localhost:8081/mcp",
301-
Group: groupName,
349+
Name: "error1",
350+
Status: runtime.WorkloadStatusError,
351+
URL: "http://localhost:8081/mcp",
352+
TransportType: types.TransportTypeSSE,
353+
Group: groupName,
302354
}
303355

304356
mockWorkloads.EXPECT().
@@ -312,9 +364,10 @@ func TestCLIBackendDiscoverer_Discover(t *testing.T) {
312364
discoverer := NewCLIBackendDiscoverer(mockWorkloads, mockGroups)
313365
backends, err := discoverer.Discover(context.Background(), groupName)
314366

315-
require.Error(t, err)
316-
assert.ErrorIs(t, err, ErrNoBackendsFound)
317-
assert.Nil(t, backends)
367+
require.NoError(t, err)
368+
assert.Len(t, backends, 2)
369+
assert.Equal(t, vmcp.BackendUnhealthy, backends[0].HealthStatus)
370+
assert.Equal(t, vmcp.BackendUnhealthy, backends[1].HealthStatus)
318371
})
319372

320373
t.Run("gracefully handles workload get failures", func(t *testing.T) {

‎pkg/vmcp/client/conversions_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ func TestContentInterfaceHandling(t *testing.T) {
178178
if textContent, ok := mcp.AsTextContent(content); ok {
179179
key := textKey
180180
if textIndex > 0 {
181-
key = "text_" + string(rune('0'+textIndex))
181+
key = fmt.Sprintf("text_%d", textIndex)
182182
}
183183
resultMap[key] = textContent.Text
184184
textIndex++
@@ -210,12 +210,12 @@ func TestContentInterfaceHandling(t *testing.T) {
210210
if textContent, ok := mcp.AsTextContent(content); ok {
211211
key := textKey
212212
if textIndex > 0 {
213-
key = "text_" + string(rune('0'+textIndex))
213+
key = fmt.Sprintf("text_%d", textIndex)
214214
}
215215
resultMap[key] = textContent.Text
216216
textIndex++
217217
} else if imageContent, ok := mcp.AsImageContent(content); ok {
218-
key := "image_" + string(rune('0'+imageIndex))
218+
key := fmt.Sprintf("image_%d", imageIndex)
219219
resultMap[key] = imageContent.Data
220220
imageIndex++
221221
}
@@ -517,7 +517,7 @@ func TestMultipleContentItemsHandling(t *testing.T) {
517517
if textContent, ok := mcp.AsTextContent(content); ok {
518518
key := textKey
519519
if textIndex > 0 {
520-
key = "text_" + string(rune('0'+textIndex))
520+
key = fmt.Sprintf("text_%d", textIndex)
521521
}
522522
resultMap[key] = textContent.Text
523523
textIndex++
@@ -549,7 +549,7 @@ func TestMultipleContentItemsHandling(t *testing.T) {
549549
imageIndex := 0
550550
for _, content := range toolResult.Content {
551551
if imageContent, ok := mcp.AsImageContent(content); ok {
552-
key := "image_" + string(rune('0'+imageIndex))
552+
key := fmt.Sprintf("image_%d", imageIndex)
553553
resultMap[key] = imageContent.Data
554554
imageIndex++
555555
}

0 commit comments

Comments
 (0)