fix: address PR review comments for subagent custom tools

- Fix onDestroy comment to reflect actual call site (Disconnect, not Destroy)
- Return slice element pointer directly in getAgentConfig to avoid copy
- Guard against empty toolCallID in onSubagentStarted to prevent map key collision
- Use non-blocking channel send in E2E test to prevent deadlock on retries
- Strengthen unsupported-tool test assertion (require non-nil content)

Author: NGUYEN Duc Trung

go/client.go

@@ -81,14 +81,14 @@ type subagentInstance struct {
 //	}
 //	defer client.Stop()
 type Client struct {
-	options          ClientOptions
-	process          *exec.Cmd
-	client           *jsonrpc2.Client
-	actualPort       int
-	actualHost       string
-	state            ConnectionState
-	sessions         map[string]*Session
-	sessionsMux      sync.Mutex
+	options     ClientOptions
+	process     *exec.Cmd
+	client      *jsonrpc2.Client
+	actualPort  int
+	actualHost  string
+	state       ConnectionState
+	sessions    map[string]*Session
+	sessionsMux sync.Mutex
 
 	// childToParent maps childSessionID → parentSessionID.
 	// Populated exclusively from authoritative protocol signals.
@@ -151,16 +151,16 @@ func NewClient(options *ClientOptions) *Client {
 	}
 
 	client := &Client{
-		options:          opts,
-		state:            StateDisconnected,
+		options:           opts,
+		state:             StateDisconnected,
 		sessions:          make(map[string]*Session),
 		childToParent:     make(map[string]string),
 		childToAgent:      make(map[string]string),
 		subagentInstances: make(map[string]map[string]*subagentInstance),
 		actualHost:        "localhost",
-		isExternalServer: false,
-		useStdio:         true,
-		autoStart:        true, // default
+		isExternalServer:  false,
+		useStdio:          true,
+		autoStart:         true, // default
 	}
 
 	if options != nil {
@@ -1569,15 +1569,19 @@ func (c *Client) onSubagentStarted(parentSessionID string, event SessionEvent) {
 	c.sessionsMux.Lock()
 	defer c.sessionsMux.Unlock()
 
-	// Track instance by toolCallID (unique per launch)
-	if c.subagentInstances[parentSessionID] == nil {
-		c.subagentInstances[parentSessionID] = make(map[string]*subagentInstance)
-	}
-	c.subagentInstances[parentSessionID][toolCallID] = &subagentInstance{
-		agentName:      agentName,
-		toolCallID:     toolCallID,
-		childSessionID: childSessionID,
-		startedAt:      event.Timestamp,
+	// Track instance by toolCallID (unique per launch).
+	// Skip tracking when toolCallID is empty — multiple launches would collide
+	// on the empty-string key and overwrite each other.
+	if toolCallID != "" {
+		if c.subagentInstances[parentSessionID] == nil {
+			c.subagentInstances[parentSessionID] = make(map[string]*subagentInstance)
+		}
+		c.subagentInstances[parentSessionID][toolCallID] = &subagentInstance{
+			agentName:      agentName,
+			toolCallID:     toolCallID,
+			childSessionID: childSessionID,
+			startedAt:      event.Timestamp,
+		}
 	}
 
 	// Eagerly map child→parent and child→agent

go/internal/e2e/subagent_tool_test.go

@@ -39,7 +39,10 @@ func TestSubagentCustomTools(t *testing.T) {
 					func(params struct {
 						Result string `json:"result" jsonschema:"The result to save"`
 					}, inv copilot.ToolInvocation) (string, error) {
-						toolInvoked <- params.Result
+						select {
+						case toolInvoked <- params.Result:
+						default:
+						}
 						return "saved: " + params.Result, nil
 					}),
 			},
@@ -127,9 +130,11 @@ func TestSubagentCustomTools(t *testing.T) {
 		if err != nil {
 			t.Fatalf("Failed to get assistant message: %v", err)
 		}
-		if answer.Data.Content != nil {
-			t.Logf("Response: %s", *answer.Data.Content)
+		if answer.Data.Content == nil {
+			t.Fatal("Expected assistant message content but got nil")
 		}
-		// The restricted_tool handler should NOT have been called (assertion in handler above)
+		t.Logf("Response: %s", *answer.Data.Content)
+		// The primary assertion is the t.Error inside restricted_tool's handler above.
+		// We don't assert on response text because LLM output is non-deterministic.
 	})
 }

go/session.go

@@ -66,7 +66,7 @@ type Session struct {
 	hooksMux           sync.RWMutex
 	transformCallbacks map[string]SectionTransformFn
 	transformMu        sync.Mutex
-	onDestroy          func() // set by Client when session is created; called by Destroy()
+	onDestroy          func()              // set by Client when session is created; called by Disconnect() (Destroy() delegates to Disconnect())
 	customAgents       []CustomAgentConfig // agent configs from SessionConfig
 
 	// eventCh serializes user event handler dispatch. dispatchEvent enqueues;
@@ -89,8 +89,7 @@ func (s *Session) WorkspacePath() string {
 func (s *Session) getAgentConfig(agentName string) *CustomAgentConfig {
 	for i := range s.customAgents {
 		if s.customAgents[i].Name == agentName {
-			config := s.customAgents[i]
-			return &config
+			return &s.customAgents[i]
 		}
 	}
 	return nil