Skip to content

feat: support custom tools in subagents#962

Open
ductrung-nguyen wants to merge 1 commit intogithub:mainfrom
ductrung-nguyen:feat/support-custom-tools-subagent
Open

feat: support custom tools in subagents#962
ductrung-nguyen wants to merge 1 commit intogithub:mainfrom
ductrung-nguyen:feat/support-custom-tools-subagent

Conversation

@ductrung-nguyen
Copy link
Copy Markdown

All three SDKs (Go, Python, Node) use per-session tool handler lookup keyed to the exact session ID. When the CLI creates child sessions for subagents, those child session IDs are never registered in the SDK's sessions map. Tool calls arriving with a child session ID fail with "unknown session <id>".

This PR only brings the ability to call custom tools for sub-agent in Go, by following the proposal in #947

@ductrung-nguyen ductrung-nguyen requested a review from a team as a code owner March 31, 2026 08:22
Copilot AI review requested due to automatic review settings March 31, 2026 08:22
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enables Go SDK sessions to handle custom tool calls originating from CLI-created subagent (child) sessions by resolving child session IDs back to the parent session and enforcing per-agent tool allowlists.

Changes:

  • Track child→parent (and child→agent) session lineage from subagent.* lifecycle events and resolve incoming RPC requests against the parent session.
  • Enforce CustomAgentConfig.Tools allowlists for tool calls coming from child sessions.
  • Add Go docs plus unit/integration tests and placeholder replay snapshots for future E2E capture.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
go/client.go Adds child-session lineage tracking, resolveSession, and allowlist enforcement for child-originated tool calls; updates request handlers to resolve via parent.
go/session.go Stores custom agent configs on the session and adds an onDestroy callback for client-side cleanup.
go/client_subagent_test.go Adds unit tests covering session resolution, allowlists, subagent tracking, cleanup, and concurrency safety.
go/internal/e2e/subagent_tool_test.go Adds integration-tagged tests exercising real CLI subagent tool invocation and deny behavior.
go/README.md Documents subagent custom tool routing and tool access control semantics.
test/snapshots/subagent_tool/subagent_invokes_parent_custom_tool.yaml Placeholder snapshot intended to be captured from a real CLI session for subagent custom tool flow.
test/snapshots/subagent_tool/subagent_denied_unlisted_tool_returns_unsupported.yaml Placeholder snapshot intended to be captured from a real CLI session for denied-tool flow.
Comments suppressed due to low confidence (3)

go/client.go:1706

  • Now that child session IDs are resolved to the parent session via resolveSession, the user-input handler invoked via session.handleUserInputRequest(...) will receive UserInputInvocation.SessionID == parentSessionID (because the invocation is built inside Session). This loses the child session context for subagent-originated prompts. Consider plumbing the original req.SessionID through so handlers can distinguish parent vs child/subagent requests.
	session, _, err := c.resolveSession(req.SessionID)
	if err != nil {
		return nil, &jsonrpc2.Error{Code: -32602, Message: err.Error()}
	}

	response, err := session.handleUserInputRequest(UserInputRequest{
		Question:      req.Question,
		Choices:       req.Choices,
		AllowFreeform: req.AllowFreeform,
	})

go/client.go:1726

  • Similar to user input: after resolving a child session to the parent, session.handleHooksInvoke(...) will build HookInvocation.SessionID from the parent session ID. That makes hook handlers unable to attribute hook invocations to the originating child/subagent session. Consider passing the original req.SessionID through (or extending the invocation type) so hook consumers can enforce policy per subagent session when needed.
	session, _, err := c.resolveSession(req.SessionID)
	if err != nil {
		return nil, &jsonrpc2.Error{Code: -32602, Message: err.Error()}
	}

	output, err := session.handleHooksInvoke(req.Type, req.Input)
	if err != nil {

go/client.go:1853

  • After resolveSession enables permission requests from child sessions, PermissionInvocation.SessionID is still populated from the resolved parent session (session.SessionID). That prevents permission handlers from knowing which child/subagent session requested the permission. Consider using the original req.SessionID (and/or adding ParentSessionID) so permission handlers can implement subagent-specific policy.
	session, _, err := c.resolveSession(req.SessionID)
	if err != nil {
		return nil, &jsonrpc2.Error{Code: -32602, Message: err.Error()}
	}

	handler := session.getPermissionHandler()
	if handler == nil {

hooksMux sync.RWMutex
transformCallbacks map[string]SectionTransformFn
transformMu sync.Mutex
onDestroy func() // set by Client when session is created; called by Destroy()
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onDestroy is documented as being called by Destroy(), but it is actually invoked from Disconnect() (and Destroy() only delegates to Disconnect()). Update the comment to reflect the actual call site to avoid confusion when reading the lifecycle logic.

Suggested change
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())

Copilot uses AI. Check for mistakes.
Comment on lines +92 to +93
config := s.customAgents[i]
return &config
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getAgentConfig returns a pointer to a local copy (config := ...; return &config). That pointer is valid, but it disconnects callers from the underlying slice element (future mutations won’t affect the stored config) and adds an avoidable allocation. Prefer returning &s.customAgents[i] (or return the value instead of a pointer) to avoid subtle bugs later.

Suggested change
config := s.customAgents[i]
return &config
return &s.customAgents[i]

Copilot uses AI. Check for mistakes.
Comment on lines +1565 to +1581
toolCallID := derefStr(event.Data.ToolCallID)
agentName := derefStr(event.Data.AgentName)
childSessionID := derefStr(event.Data.RemoteSessionID)

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,
}
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onSubagentStarted uses toolCallID (which is optional in the event schema) as the map key without guarding against it being empty. If the server omits toolCallId, multiple launches will collide on the empty-string key and overwrite tracking state. Consider skipping subagentInstances tracking when toolCallID == "" or generating a unique key so instances can’t collide.

Copilot uses AI. Check for mistakes.
func(params struct {
Result string `json:"result" jsonschema:"The result to save"`
}, inv copilot.ToolInvocation) (string, error) {
toolInvoked <- params.Result
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tool handler sends into toolInvoked using a buffered channel of size 1. If the model retries or invokes the tool more than once, the second send will block and can deadlock the test run. Consider increasing the buffer, draining in a goroutine, or using a non-blocking send/select to make the test resilient to multiple invocations.

Suggested change
toolInvoked <- params.Result
select {
case toolInvoked <- params.Result:
default:
}

Copilot uses AI. Check for mistakes.
Comment on lines +130 to +132
if answer.Data.Content != nil {
t.Logf("Response: %s", *answer.Data.Content)
}
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test case name says "returns unsupported" but the assertions only ensure the restricted tool handler wasn’t called; it doesn’t verify that the SDK returned the expected unsupported-tool result/message. Add an assertion (e.g., checking the final assistant message contains the expected error, or inspecting events/tool results) so the test actually validates the intended behavior.

Suggested change
if answer.Data.Content != nil {
t.Logf("Response: %s", *answer.Data.Content)
}
if answer.Data.Content == nil {
t.Fatalf("Expected assistant message content but got nil")
}
content := strings.ToLower(*answer.Data.Content)
t.Logf("Response: %s", content)
if !strings.Contains(content, "unsupported") {
t.Fatalf("Expected assistant response to indicate unsupported tool, got: %q", content)
}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants