From 83764f4c6f1739277c709a0039a4b136e8e1c2cb Mon Sep 17 00:00:00 2001 From: Molecule AI Fullstack Engineer Date: Tue, 12 May 2026 18:13:47 +0000 Subject: [PATCH 1/3] fix(handlers/discovery): nil-guard in filterPeersByQuery + test coverage for #730 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes a type-assertion panic when a workspace has an empty role string. queryPeerMaps explicitly sets peer["role"] = nil for empty-string roles (discovery.go:340), and filterPeersByQuery did p["role"].(string) without guarding for nil. The fix uses the comma-ok idiom so nil returns "" and no match occurs — the correct behaviour. Test files added (all pure functions, no DB/side effects): - discovery_filter_test.go (12 cases): nil-role/name guard regression, empty query no-op, whitespace trimming, name/role matching, case insensitivity, empty peers, partial matches. - org_helpers_walk_test.go (16 cases): walkOrgWorkspaceNames (empty tree, single node, nested, deeply nested, skips empty names, spawning:false still walks), resolveProvisionConcurrency (default, valid int, zero unlimited, negative falls back, non-integer falls back, whitespace), errString (nil, non-nil, empty). - delegation_extract_response_text_test.go (17 cases): extractResponseText covers all code paths — parts text kind, non-text kind, nil text, empty parts/artifacts, artifact parts, non-map elements, kind not string, no result, result not map, non-JSON fallback, nil body. Co-Authored-By: Claude Opus 4.7 --- .../delegation_extract_response_text_test.go | 224 ++++++++++++++++++ .../internal/handlers/discovery.go | 8 +- .../handlers/discovery_filter_test.go | 160 +++++++++++++ .../handlers/org_helpers_walk_test.go | 191 +++++++++++++++ 4 files changed, 581 insertions(+), 2 deletions(-) create mode 100644 workspace-server/internal/handlers/delegation_extract_response_text_test.go create mode 100644 workspace-server/internal/handlers/discovery_filter_test.go create mode 100644 workspace-server/internal/handlers/org_helpers_walk_test.go diff --git a/workspace-server/internal/handlers/delegation_extract_response_text_test.go b/workspace-server/internal/handlers/delegation_extract_response_text_test.go new file mode 100644 index 00000000..a694b322 --- /dev/null +++ b/workspace-server/internal/handlers/delegation_extract_response_text_test.go @@ -0,0 +1,224 @@ +package handlers + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" +) + +// extractResponseText tests — walks A2A JSON-RPC response bodies and +// returns the first text part, falling back to raw body on parse failures. + +func TestExtractResponseText_PartsWithTextKind(t *testing.T) { + resp := map[string]interface{}{ + "result": map[string]interface{}{ + "parts": []interface{}{ + map[string]interface{}{"kind": "text", "text": "hello world"}, + map[string]interface{}{"kind": "text", "text": "second part"}, + }, + }, + } + body, _ := json.Marshal(resp) + assert.Equal(t, "hello world", extractResponseText(body)) +} + +func TestExtractResponseText_PartNotTextKind(t *testing.T) { + resp := map[string]interface{}{ + "result": map[string]interface{}{ + "parts": []interface{}{ + map[string]interface{}{"kind": "image", "data": "base64..."}, + map[string]interface{}{"kind": "text", "text": "visible"}, + }, + }, + } + body, _ := json.Marshal(resp) + assert.Equal(t, "visible", extractResponseText(body)) +} + +func TestExtractResponseText_PartsEmpty(t *testing.T) { + // Empty parts array — falls through to artifacts, then raw body + resp := map[string]interface{}{ + "result": map[string]interface{}{ + "parts": []interface{}{}, + "artifacts": []interface{}{}, + }, + } + body, _ := json.Marshal(resp) + // Falls through to raw body (which is the JSON string) + result := extractResponseText(body) + assert.NotEmpty(t, result) +} + +func TestExtractResponseText_ArtifactPartsWithText(t *testing.T) { + resp := map[string]interface{}{ + "result": map[string]interface{}{ + "parts": []interface{}{}, + "artifacts": []interface{}{ + map[string]interface{}{ + "kind": "file", + "parts": []interface{}{ + map[string]interface{}{"kind": "text", "text": "artifact text"}, + }, + }, + }, + }, + } + body, _ := json.Marshal(resp) + assert.Equal(t, "artifact text", extractResponseText(body)) +} + +func TestExtractResponseText_ArtifactPartNotTextKind(t *testing.T) { + resp := map[string]interface{}{ + "result": map[string]interface{}{ + "parts": []interface{}{}, + "artifacts": []interface{}{ + map[string]interface{}{ + "kind": "code", + "parts": []interface{}{ + map[string]interface{}{"kind": "image", "data": "..."}, + map[string]interface{}{"kind": "text", "text": "code comment"}, + }, + }, + }, + }, + } + body, _ := json.Marshal(resp) + assert.Equal(t, "code comment", extractResponseText(body)) +} + +func TestExtractResponseText_ArtifactsEmpty(t *testing.T) { + resp := map[string]interface{}{ + "result": map[string]interface{}{ + "parts": []interface{}{}, + "artifacts": []interface{}{}, + }, + } + body, _ := json.Marshal(resp) + result := extractResponseText(body) + // Falls back to raw body + assert.Equal(t, string(body), result) +} + +func TestExtractResponseText_NoResult(t *testing.T) { + // No "result" key at all — falls back to raw body + body := []byte(`{"error": {"code": -32600, "message": "Invalid Request"}}`) + result := extractResponseText(body) + assert.Equal(t, string(body), result) +} + +func TestExtractResponseText_ResultNotMap(t *testing.T) { + // result is a string, not a map — falls back to raw body + body := []byte(`{"result": "just a string"}`) + result := extractResponseText(body) + assert.Equal(t, string(body), result) +} + +func TestExtractResponseText_NonJSONBody(t *testing.T) { + // Non-JSON bytes — returns the raw string + body := []byte("plain text response, not JSON at all") + result := extractResponseText(body) + assert.Equal(t, "plain text response, not JSON at all", result) +} + +func TestExtractResponseText_PartWithNilText(t *testing.T) { + // Text field is nil — kind is "text" but text is nil, should skip + resp := map[string]interface{}{ + "result": map[string]interface{}{ + "parts": []interface{}{ + map[string]interface{}{"kind": "text", "text": nil}, + map[string]interface{}{"kind": "text", "text": "found"}, + }, + }, + } + body, _ := json.Marshal(resp) + assert.Equal(t, "found", extractResponseText(body)) +} + +func TestExtractResponseText_ArtifactPartWithNilText(t *testing.T) { + resp := map[string]interface{}{ + "result": map[string]interface{}{ + "parts": []interface{}{}, + "artifacts": []interface{}{ + map[string]interface{}{ + "parts": []interface{}{ + map[string]interface{}{"kind": "text", "text": nil}, + map[string]interface{}{"kind": "text", "text": "artifact-found"}, + }, + }, + }, + }, + } + body, _ := json.Marshal(resp) + assert.Equal(t, "artifact-found", extractResponseText(body)) +} + +func TestExtractResponseText_PartsWithNonMapElement(t *testing.T) { + // parts contains a non-map element — should be skipped gracefully + resp := map[string]interface{}{ + "result": map[string]interface{}{ + "parts": []interface{}{ + "not a map", + 123, + nil, + map[string]interface{}{"kind": "text", "text": "parsed"}, + }, + }, + } + body, _ := json.Marshal(resp) + assert.Equal(t, "parsed", extractResponseText(body)) +} + +func TestExtractResponseText_ArtifactWithNonMapElement(t *testing.T) { + resp := map[string]interface{}{ + "result": map[string]interface{}{ + "parts": []interface{}{}, + "artifacts": []interface{}{ + "not a map", + nil, + map[string]interface{}{ + "parts": []interface{}{ + "not a map", + map[string]interface{}{"kind": "text", "text": "safe"}, + }, + }, + }, + }, + } + body, _ := json.Marshal(resp) + assert.Equal(t, "safe", extractResponseText(body)) +} + +func TestExtractResponseText_PartKindNotString(t *testing.T) { + // kind is an integer, not a string — should be skipped + resp := map[string]interface{}{ + "result": map[string]interface{}{ + "parts": []interface{}{ + map[string]interface{}{"kind": 123, "text": "ignored"}, + map[string]interface{}{"kind": "text", "text": "found"}, + }, + }, + } + body, _ := json.Marshal(resp) + assert.Equal(t, "found", extractResponseText(body)) +} + +func TestExtractResponseText_EmptyResponse(t *testing.T) { + body := []byte("{}") + result := extractResponseText(body) + // Falls back to raw "{}" + assert.Equal(t, "{}", result) +} + +func TestExtractResponseText_NilBody(t *testing.T) { + // nil byte slice — string(nil) = "" + result := extractResponseText(nil) + assert.Equal(t, "", result) +} + +func TestExtractResponseText_WhitespaceBody(t *testing.T) { + body := []byte(" \n\t ") + result := extractResponseText(body) + // Unmarshals to empty map, no result, returns raw string + assert.Equal(t, " \n\t ", result) +} diff --git a/workspace-server/internal/handlers/discovery.go b/workspace-server/internal/handlers/discovery.go index 79315016..48a3131a 100644 --- a/workspace-server/internal/handlers/discovery.go +++ b/workspace-server/internal/handlers/discovery.go @@ -292,8 +292,12 @@ func filterPeersByQuery(peers []map[string]interface{}, q string) []map[string]i needle := strings.ToLower(q) out := make([]map[string]interface{}, 0, len(peers)) for _, p := range peers { - name := p["name"].(string) - role := p["role"].(string) + // Comma-ok idiom: nil map values return (nil, false), protecting + // against type-assertion panics when queryPeerMaps explicitly sets + // role=nil for empty-string roles (discovery.go:340). Also guards + // against nil name if the DB returns NULL. + name, _ := p["name"].(string) + role, _ := p["role"].(string) if strings.Contains(strings.ToLower(name), needle) || strings.Contains(strings.ToLower(role), needle) { out = append(out, p) diff --git a/workspace-server/internal/handlers/discovery_filter_test.go b/workspace-server/internal/handlers/discovery_filter_test.go new file mode 100644 index 00000000..7570c751 --- /dev/null +++ b/workspace-server/internal/handlers/discovery_filter_test.go @@ -0,0 +1,160 @@ +package handlers + +import ( + "testing" +) + +// filterPeersByQuery tests — nil-safe role/name filtering for peer discovery. + +func TestFilterPeersByQuery_EmptyQueryNoOp(t *testing.T) { + peers := []map[string]interface{}{ + {"name": "foo", "role": "bar"}, + {"name": "baz", "role": "qux"}, + } + result := filterPeersByQuery(peers, "") + if len(result) != 2 { + t.Errorf("empty query: expected 2, got %d", len(result)) + } +} + +func TestFilterPeersByQuery_WhitespaceQueryNoOp(t *testing.T) { + peers := []map[string]interface{}{ + {"name": "foo", "role": "bar"}, + } + result := filterPeersByQuery(peers, " ") + if len(result) != 1 { + t.Errorf("whitespace-only query: expected 1, got %d", len(result)) + } +} + +func TestFilterPeersByQuery_MatchName(t *testing.T) { + peers := []map[string]interface{}{ + {"name": "backend-agent", "role": "sre"}, + {"name": "frontend-agent", "role": "ui"}, + } + result := filterPeersByQuery(peers, "backend") + if len(result) != 1 || result[0]["name"] != "backend-agent" { + t.Errorf("expected backend-agent, got %v", result) + } +} + +func TestFilterPeersByQuery_MatchRole(t *testing.T) { + peers := []map[string]interface{}{ + {"name": "agent-alpha", "role": "security engineer"}, + {"name": "agent-beta", "role": "devops"}, + } + result := filterPeersByQuery(peers, "engineer") + if len(result) != 1 || result[0]["name"] != "agent-alpha" { + t.Errorf("expected agent-alpha, got %v", result) + } +} + +func TestFilterPeersByQuery_CaseInsensitive(t *testing.T) { + peers := []map[string]interface{}{ + {"name": "AgentX", "role": "SRE"}, + } + result := filterPeersByQuery(peers, "AGENTx") + if len(result) != 1 { + t.Errorf("expected 1 match (case-insensitive), got %d", len(result)) + } +} + +func TestFilterPeersByQuery_NilRoleNoPanic(t *testing.T) { + // This is the regression case for #730: queryPeerMaps explicitly sets + // peer["role"] = nil when the DB role is empty string. Before the fix, + // p["role"].(string) panics on nil. After the fix, it returns "" and + // no match occurs — which is the correct behaviour. + defer func() { + if r := recover(); r != nil { + t.Errorf("filterPeersByQuery panicked on nil role: %v", r) + } + }() + peers := []map[string]interface{}{ + {"name": "some-agent", "role": nil}, + } + result := filterPeersByQuery(peers, "some-agent") + if len(result) != 1 { + t.Errorf("expected 1 match by name, got %d", len(result)) + } +} + +func TestFilterPeersByQuery_NilRoleQueryNoMatch(t *testing.T) { + // When role is nil and query does not match name, nothing matches. + defer func() { + if r := recover(); r != nil { + t.Errorf("filterPeersByQuery panicked on nil role: %v", r) + } + }() + peers := []map[string]interface{}{ + {"name": "agent-alpha", "role": nil}, + } + result := filterPeersByQuery(peers, "no-match") + if len(result) != 0 { + t.Errorf("expected 0 matches, got %d", len(result)) + } +} + +func TestFilterPeersByQuery_NilNameNoPanic(t *testing.T) { + // Defensive check: name could also theoretically be nil. + defer func() { + if r := recover(); r != nil { + t.Errorf("filterPeersByQuery panicked on nil name: %v", r) + } + }() + peers := []map[string]interface{}{ + {"name": nil, "role": "sre"}, + } + result := filterPeersByQuery(peers, "sre") + if len(result) != 1 { + t.Errorf("expected 1 match by role, got %d", len(result)) + } +} + +func TestFilterPeersByQuery_BothNilNoPanic(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Errorf("filterPeersByQuery panicked on nil name+role: %v", r) + } + }() + peers := []map[string]interface{}{ + {"name": nil, "role": nil}, + } + result := filterPeersByQuery(peers, "") + if len(result) != 1 { + t.Errorf("empty query with nil name/role: expected 1, got %d", len(result)) + } + result = filterPeersByQuery(peers, "anything") + if len(result) != 0 { + t.Errorf("non-empty query with nil name/role: expected 0, got %d", len(result)) + } +} + +func TestFilterPeersByQuery_NoMatches(t *testing.T) { + peers := []map[string]interface{}{ + {"name": "alpha", "role": "beta"}, + {"name": "gamma", "role": "delta"}, + } + result := filterPeersByQuery(peers, "zzz") + if len(result) != 0 { + t.Errorf("expected 0, got %d", len(result)) + } +} + +func TestFilterPeersByQuery_EmptyPeers(t *testing.T) { + result := filterPeersByQuery([]map[string]interface{}{}, "query") + if len(result) != 0 { + t.Errorf("empty peers: expected 0, got %d", len(result)) + } +} + +func TestFilterPeersByQuery_MultipleMatches(t *testing.T) { + peers := []map[string]interface{}{ + {"name": "backend-alpha", "role": "eng"}, + {"name": "backend-beta", "role": "eng"}, + {"name": "frontend", "role": "ui"}, + } + result := filterPeersByQuery(peers, "backend") + if len(result) != 2 { + t.Errorf("expected 2 backend matches, got %d", len(result)) + } +} diff --git a/workspace-server/internal/handlers/org_helpers_walk_test.go b/workspace-server/internal/handlers/org_helpers_walk_test.go new file mode 100644 index 00000000..d936c8ce --- /dev/null +++ b/workspace-server/internal/handlers/org_helpers_walk_test.go @@ -0,0 +1,191 @@ +package handlers + +import ( + "errors" + "os" + "testing" + + "github.com/stretchr/testify/assert" +) + +// walkOrgWorkspaceNames tests — recursive collection of non-empty workspace names. + +func TestWalkOrgWorkspaceNames_EmptySlice(t *testing.T) { + var names []string + walkOrgWorkspaceNames([]OrgWorkspace{}, &names) + assert.Empty(t, names) +} + +func TestWalkOrgWorkspaceNames_SingleNode(t *testing.T) { + var names []string + walkOrgWorkspaceNames([]OrgWorkspace{{Name: "my-workspace"}}, &names) + assert.Equal(t, []string{"my-workspace"}, names) +} + +func TestWalkOrgWorkspaceNames_SingleNodeEmptyName(t *testing.T) { + var names []string + walkOrgWorkspaceNames([]OrgWorkspace{{Name: ""}}, &names) + assert.Empty(t, names) +} + +func TestWalkOrgWorkspaceNames_NestedChildren(t *testing.T) { + var names []string + tree := []OrgWorkspace{ + { + Name: "parent", + Children: []OrgWorkspace{ + {Name: "child-a"}, + {Name: "child-b"}, + }, + }, + } + walkOrgWorkspaceNames(tree, &names) + assert.Equal(t, []string{"parent", "child-a", "child-b"}, names) +} + +func TestWalkOrgWorkspaceNames_DeeplyNested(t *testing.T) { + var names []string + tree := []OrgWorkspace{ + { + Name: "level0", + Children: []OrgWorkspace{ + { + Name: "level1", + Children: []OrgWorkspace{ + { + Name: "level2", + Children: []OrgWorkspace{ + {Name: "level3"}, + }, + }, + }, + }, + }, + }, + } + walkOrgWorkspaceNames(tree, &names) + assert.Equal(t, []string{"level0", "level1", "level2", "level3"}, names) +} + +func TestWalkOrgWorkspaceNames_SkipsEmptyNames(t *testing.T) { + var names []string + tree := []OrgWorkspace{ + {Name: "a"}, + {Name: ""}, + {Name: "b"}, + } + walkOrgWorkspaceNames(tree, &names) + assert.Equal(t, []string{"a", "b"}, names) +} + +func TestWalkOrgWorkspaceNames_Siblings(t *testing.T) { + var names []string + tree := []OrgWorkspace{ + {Name: "team"}, + {Name: "alpha"}, + {Name: "beta"}, + } + walkOrgWorkspaceNames(tree, &names) + assert.Equal(t, []string{"team", "alpha", "beta"}, names) +} + +func TestWalkOrgWorkspaceNames_MultipleRoots(t *testing.T) { + var names []string + tree := []OrgWorkspace{ + {Name: "root-a", Children: []OrgWorkspace{{Name: "child-a"}}}, + {Name: "root-b", Children: []OrgWorkspace{{Name: "child-b"}}}, + } + walkOrgWorkspaceNames(tree, &names) + assert.Equal(t, []string{"root-a", "child-a", "root-b", "child-b"}, names) +} + +func TestWalkOrgWorkspaceNames_SpawningFalseStillWalks(t *testing.T) { + // The comment in the source is explicit: spawning:false subtrees are + // still walked. Empty names within those subtrees are still skipped. + var names []string + yes := true + no := false + tree := []OrgWorkspace{ + { + Name: "parent", + Children: []OrgWorkspace{ + {Name: "spawning-child", Spawning: &yes}, + {Name: "non-spawning-child", Spawning: &no}, + {Name: ""}, + }, + }, + } + walkOrgWorkspaceNames(tree, &names) + assert.Equal(t, []string{"parent", "spawning-child", "non-spawning-child"}, names) +} + +// resolveProvisionConcurrency tests — env-var parsing with sensible fallback. + +func TestResolveProvisionConcurrency_Default(t *testing.T) { + os.Unsetenv("MOLECULE_PROVISION_CONCURRENCY") + defer os.Unsetenv("MOLECULE_PROVISION_CONCURRENCY") + val := resolveProvisionConcurrency() + assert.Equal(t, defaultProvisionConcurrency, val) +} + +func TestResolveProvisionConcurrency_ValidPositiveInt(t *testing.T) { + os.Setenv("MOLECULE_PROVISION_CONCURRENCY", "5") + defer os.Unsetenv("MOLECULE_PROVISION_CONCURRENCY") + val := resolveProvisionConcurrency() + assert.Equal(t, 5, val) +} + +func TestResolveProvisionConcurrency_ZeroUnlimited(t *testing.T) { + os.Setenv("MOLECULE_PROVISION_CONCURRENCY", "0") + defer os.Unsetenv("MOLECULE_PROVISION_CONCURRENCY") + val := resolveProvisionConcurrency() + // Zero is mapped to 1<<20 (unlimited semantics with finite cap) + assert.Equal(t, 1<<20, val) +} + +func TestResolveProvisionConcurrency_NegativeFallsBack(t *testing.T) { + os.Setenv("MOLECULE_PROVISION_CONCURRENCY", "-1") + defer os.Unsetenv("MOLECULE_PROVISION_CONCURRENCY") + val := resolveProvisionConcurrency() + assert.Equal(t, defaultProvisionConcurrency, val) +} + +func TestResolveProvisionConcurrency_NonIntegerFallsBack(t *testing.T) { + os.Setenv("MOLECULE_PROVISION_CONCURRENCY", "not-a-number") + defer os.Unsetenv("MOLECULE_PROVISION_CONCURRENCY") + val := resolveProvisionConcurrency() + assert.Equal(t, defaultProvisionConcurrency, val) +} + +func TestResolveProvisionConcurrency_WhitespaceOnly(t *testing.T) { + os.Setenv("MOLECULE_PROVISION_CONCURRENCY", " ") + defer os.Unsetenv("MOLECULE_PROVISION_CONCURRENCY") + val := resolveProvisionConcurrency() + assert.Equal(t, defaultProvisionConcurrency, val) +} + +func TestResolveProvisionConcurrency_LargeValue(t *testing.T) { + os.Setenv("MOLECULE_PROVISION_CONCURRENCY", "10000") + defer os.Unsetenv("MOLECULE_PROVISION_CONCURRENCY") + val := resolveProvisionConcurrency() + assert.Equal(t, 10000, val) +} + +// errString tests — nil-safe error-to-string wrapper. + +func TestErrString_NilError(t *testing.T) { + result := errString(nil) + assert.Equal(t, "", result) +} + +func TestErrString_WithError(t *testing.T) { + err := errors.New("something went wrong") + result := errString(err) + assert.Equal(t, "something went wrong", result) +} + +func TestErrString_EmptyError(t *testing.T) { + err := errors.New("") + result := errString(err) + assert.Equal(t, "", result) +} -- 2.52.0 From 6fc97a81e1cf2935022223e00135e43f0c0a360e Mon Sep 17 00:00:00 2001 From: fullstack-engineer Date: Tue, 12 May 2026 19:30:31 +0000 Subject: [PATCH 2/3] ci: trigger CI rerun [empty commit] -- 2.52.0 From df5507cf40e0ec65322d03418f2d2a959eccccfb Mon Sep 17 00:00:00 2001 From: core-devops Date: Tue, 12 May 2026 20:50:58 +0000 Subject: [PATCH 3/3] ci: rerun after mc#724 all-required fix lands -- 2.52.0