Add the ability to click through to sub-agent session#1362
Add the ability to click through to sub-agent session#1362dobesv wants to merge 1 commit intokagent-dev:mainfrom
Conversation
When debugging agent behavior it is very useful to be able to drill down into the sub-agent activity and what tool calls they had - or even drill down into *their* sub-agent calls, and so on. I find this feature very useful in agent development with sub-agents and sub-sub-agents. I'm not super happy with this implementation. It does basically work, though (at least it does in my main branch with all my customizations in it)
There was a problem hiding this comment.
Pull request overview
This PR adds the ability to navigate from a parent agent's function call display to the corresponding sub-agent's session in the UI. This is achieved by:
- Propagating parent function call IDs through the A2A protocol metadata
- Storing this metadata in the database via session events
- Adding a lookup endpoint to find sessions by parent function call ID
- Creating UI components to navigate to sub-agent sessions
Changes:
- Added SubAgentSessionPlugin to capture and embed A2A metadata (context_id, task_id) in agent tool results
- Implemented database lookup functionality to find sessions by parent function call ID
- Created new UI route
/agents/[namespace]/[name]/function-calls/[id]to display sub-agent sessions - Modified AgentCallDisplay to generate clickable links for agent tool calls
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
ui/src/components/chat/AgentCallDisplay.tsx |
Added regex pattern to detect agent tools and generate clickable links to sub-agent sessions |
ui/src/app/agents/[namespace]/[name]/layout.tsx |
New layout component for the agent namespace/name route structure |
ui/src/app/agents/[namespace]/[name]/function-calls/[id]/page.tsx |
New page component that looks up and displays sub-agent sessions by function call ID |
ui/src/app/actions/sessions.ts |
Added findSessionByFunctionCallId function to call the new backend endpoint |
python/packages/kagent-adk/src/kagent/adk/_sub_agent_session_plugin.py |
New plugin that captures A2A metadata and embeds it in tool results using contextvars |
python/packages/kagent-adk/src/kagent/adk/types.py |
Added parent context metadata provider to inject function call IDs into A2A requests |
python/packages/kagent-adk/src/kagent/adk/converters/part_converter.py |
Modified to extract and clean a2a metadata keys from function responses |
python/packages/kagent-adk/src/kagent/adk/_agent_executor.py |
Added code to store A2A request metadata in session state |
python/packages/kagent-adk/src/kagent/adk/cli.py |
Updated plugin initialization to always include SubAgentSessionPlugin |
python/packages/kagent-adk/tests/unittests/test_agent_tool.py |
Comprehensive test coverage for SubAgentSessionPlugin and metadata extraction |
go/pkg/database/client.go |
Added FindSessionByParentFunctionCallID method to database interface |
go/internal/database/client.go |
Implemented session lookup using LIKE query on event data |
go/internal/database/fake/client.go |
Implemented fake client version for testing |
go/internal/httpserver/server.go |
Added new route for session lookup endpoint |
go/internal/httpserver/handlers/sessions.go |
Added HandleFindSessionByFunctionCall handler |
go/internal/httpserver/handlers/sessions_test.go |
Added test coverage for new endpoint |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useEffect(() => { | ||
| let cancelled = false; | ||
| async function lookup() { | ||
| const result = await findSessionByFunctionCallId(id); | ||
| if (cancelled) return; | ||
| if (result.data?.id) { | ||
| setSessionId(result.data.id); | ||
| } else { | ||
| setNotFound(true); | ||
| } | ||
| setLoading(false); | ||
| } | ||
| lookup(); | ||
| return () => { cancelled = true; }; | ||
| }, [id]); |
There was a problem hiding this comment.
The cleanup function sets cancelled = true but there's a potential race condition: if the async lookup() function has already read cancelled as false and is in the middle of executing, it will still call the state setters. While React batches updates and this is generally safe, consider wrapping the state updates in a check like if (!cancelled) { setSessionId(...); } for more defensive programming.
| searchPattern := fmt.Sprintf("%%\"parent_function_call_id\"%%\"%s\"%%", functionCallID) | ||
| err := c.db.Where("data LIKE ?", searchPattern).First(&event).Error |
There was a problem hiding this comment.
The function_call_id parameter is directly embedded into a SQL LIKE pattern without proper sanitization. While Go's database/sql package provides protection against SQL injection for parameterized queries, special characters in the function call ID (like %, _, etc.) could cause unexpected matching behavior in the LIKE pattern. Consider escaping these special LIKE characters or validating the input format.
| async def on_event_callback( | ||
| self, | ||
| *, | ||
| invocation_context: InvocationContext, | ||
| event: Event, | ||
| ) -> Optional[Event]: | ||
| if not event.custom_metadata: | ||
| return None | ||
|
|
||
| tc = _current_tool_call.get(None) | ||
| if tc is None: | ||
| return None | ||
|
|
||
| context_id = event.custom_metadata.get("a2a:context_id") | ||
| task_id = event.custom_metadata.get("a2a:task_id") | ||
|
|
||
| if not context_id and not task_id: | ||
| return None | ||
|
|
||
| if context_id and not tc.captured_context_id: | ||
| tc.captured_context_id = context_id | ||
| if task_id and not tc.captured_task_id: | ||
| tc.captured_task_id = task_id | ||
|
|
||
| return None |
There was a problem hiding this comment.
The contextvars mechanism relies on proper async context propagation, but there's no defensive handling if the context is lost or if before_tool_callback is not called before on_event_callback. If the callbacks are invoked out of order or if the context is not properly propagated in certain async scenarios, the plugin could fail silently. Consider adding logging or defensive checks to detect and handle these edge cases.
| data = part.function_response.model_dump(by_alias=True, exclude_none=True) | ||
| metadata = { | ||
| get_kagent_metadata_key(A2A_DATA_PART_METADATA_TYPE_KEY): A2A_DATA_PART_METADATA_TYPE_FUNCTION_RESPONSE | ||
| } | ||
| # Extract embedded sub-agent A2A metadata (injected by SubAgentSessionPlugin) | ||
| response = data.get("response", {}) | ||
| if isinstance(response, dict): | ||
| sub_ctx_id = response.pop("a2a:context_id", None) | ||
| sub_task_id = response.pop("a2a:task_id", None) | ||
| if sub_ctx_id: | ||
| metadata["a2a:context_id"] = sub_ctx_id | ||
| if sub_task_id: | ||
| metadata["a2a:task_id"] = sub_task_id |
There was a problem hiding this comment.
The code uses response.pop() to remove the a2a metadata keys from the response dict, which modifies the original data structure. This is the intended behavior to clean the metadata from the response before storing it, but if the response dict is used elsewhere after this conversion, it could lead to unexpected behavior. Consider adding a comment explaining that this intentionally mutates the input data, or create a copy of the response dict if mutation could be problematic.
| // Use a wildcard between the key and value to handle potential whitespace (e.g. ": " vs ":") | ||
| searchPattern := fmt.Sprintf("%%\"parent_function_call_id\"%%\"%s\"%%", functionCallID) | ||
| err := c.db.Where("data LIKE ?", searchPattern).First(&event).Error |
There was a problem hiding this comment.
The string pattern matching approach for JSON data is fragile and could produce false positives if the function call ID appears in other parts of the JSON (e.g., in a string value or different field). The pattern searches for any occurrence of the ID in the Data field. Consider using JSON extraction functions provided by the database (e.g., PostgreSQL's ->> operator or MySQL's JSON_EXTRACT) for more robust and accurate JSON field matching.
| // Use a wildcard between the key and value to handle potential whitespace (e.g. ": " vs ":") | |
| searchPattern := fmt.Sprintf("%%\"parent_function_call_id\"%%\"%s\"%%", functionCallID) | |
| err := c.db.Where("data LIKE ?", searchPattern).First(&event).Error | |
| // Use JSON field extraction to precisely match the parent_function_call_id value | |
| err := c.db.Where("data->>'parent_function_call_id' = ?", functionCallID).First(&event).Error |
| return ( | ||
| <ChatInterface | ||
| selectedAgentName={name} | ||
| selectedNamespace={namespace} | ||
| sessionId={sessionId!} |
There was a problem hiding this comment.
The non-null assertion operator (!) is used on sessionId without a runtime check. While the code flow ensures sessionId is not null when reaching this point (because of the prior notFound and loading checks), this creates a potential runtime error if the logic changes. Consider using a defensive check or TypeScript's nullish coalescing to make the code more robust.
| return ( | |
| <ChatInterface | |
| selectedAgentName={name} | |
| selectedNamespace={namespace} | |
| sessionId={sessionId!} | |
| if (!sessionId) { | |
| return null; | |
| } | |
| return ( | |
| <ChatInterface | |
| selectedAgentName={name} | |
| selectedNamespace={namespace} | |
| sessionId={sessionId} |
| if isinstance(result, dict): | ||
| if tc.captured_context_id: | ||
| result["a2a:context_id"] = tc.captured_context_id | ||
| if tc.captured_task_id: | ||
| result["a2a:task_id"] = tc.captured_task_id | ||
| return result |
There was a problem hiding this comment.
The plugin modifies the result dictionary by adding a2a metadata keys, which could potentially conflict with existing keys in the result if a tool legitimately returns data with keys named "a2a:context_id" or "a2a:task_id". While these are namespaced keys unlikely to collide, consider documenting this behavior or adding a check to warn if these keys already exist in the result.
| // Use a wildcard between the key and value to handle potential whitespace (e.g. ": " vs ":") | ||
| searchPattern := fmt.Sprintf("%%\"parent_function_call_id\"%%\"%s\"%%", functionCallID) | ||
| err := c.db.Where("data LIKE ?", searchPattern).First(&event).Error |
There was a problem hiding this comment.
The LIKE query pattern may have performance implications on large datasets as it performs a full table scan. Consider adding a database index on the Event.Data column if this functionality is used frequently, or consider storing parent_function_call_id in a separate indexed column for more efficient lookups.
| // Use a wildcard between the key and value to handle potential whitespace (e.g. ": " vs ":") | |
| searchPattern := fmt.Sprintf("%%\"parent_function_call_id\"%%\"%s\"%%", functionCallID) | |
| err := c.db.Where("data LIKE ?", searchPattern).First(&event).Error | |
| // Query the JSON field directly for the parent_function_call_id to avoid a LIKE scan. | |
| err := c.db.Where("JSON_EXTRACT(data, '$.parent_function_call_id') = ?", functionCallID).First(&event).Error |
| searchPattern1 := fmt.Sprintf(`"parent_function_call_id":"%s"`, functionCallID) | ||
| searchPattern2 := fmt.Sprintf(`"parent_function_call_id": "%s"`, functionCallID) | ||
|
|
||
| for _, event := range c.events { | ||
| if strings.Contains(event.Data, searchPattern1) || strings.Contains(event.Data, searchPattern2) { | ||
| for _, session := range c.sessions { | ||
| if session.ID == event.SessionID { | ||
| return session, nil | ||
| } | ||
| } | ||
| return nil, gorm.ErrRecordNotFound |
There was a problem hiding this comment.
The fake client implementation uses simple string matching which has the same issue as the real implementation - it could match the function call ID anywhere in the JSON data, not just in the parent_function_call_id field. Additionally, the dual pattern matching (with and without space after colon) is a workaround that suggests the data format is inconsistent. Consider using proper JSON parsing for more reliable matching.
| searchPattern1 := fmt.Sprintf(`"parent_function_call_id":"%s"`, functionCallID) | |
| searchPattern2 := fmt.Sprintf(`"parent_function_call_id": "%s"`, functionCallID) | |
| for _, event := range c.events { | |
| if strings.Contains(event.Data, searchPattern1) || strings.Contains(event.Data, searchPattern2) { | |
| for _, session := range c.sessions { | |
| if session.ID == event.SessionID { | |
| return session, nil | |
| } | |
| } | |
| return nil, gorm.ErrRecordNotFound | |
| for _, event := range c.events { | |
| var payload map[string]interface{} | |
| if err := json.Unmarshal([]byte(event.Data), &payload); err != nil { | |
| // If the event data is not valid JSON, skip this event. | |
| continue | |
| } | |
| if v, ok := payload["parent_function_call_id"]; ok { | |
| if id, ok := v.(string); ok && id == functionCallID { | |
| for _, session := range c.sessions { | |
| if session.ID == event.SessionID { | |
| return session, nil | |
| } | |
| } | |
| return nil, gorm.ErrRecordNotFound | |
| } |
| const AGENT_TOOL_NAME_RE = /^(.+)__NS__(.+)$/; | ||
|
|
||
| const AgentCallDisplay = ({ call, result, status = "requested", isError = false }: AgentCallDisplayProps) => { | ||
| const [areInputsExpanded, setAreInputsExpanded] = useState(false); | ||
| const [areResultsExpanded, setAreResultsExpanded] = useState(false); | ||
|
|
||
| const agentDisplay = useMemo(() => convertToUserFriendlyName(call.name), [call.name]); | ||
| const hasResult = result !== undefined; | ||
|
|
||
| const agentMatch = call.name.match(AGENT_TOOL_NAME_RE); | ||
| const functionCallLink = agentMatch | ||
| ? `/agents/${agentMatch[1].replace(/_/g, "-")}/${agentMatch[2].replace(/_/g, "-")}/function-calls/${call.id}` | ||
| : null; |
There was a problem hiding this comment.
The regex pattern assumes agent tool names follow the format "namespace__NS__name" but there is no validation or error handling for cases where the agent name doesn't match this pattern. The code silently returns null for the link, which is correct behavior, but consider adding a comment explaining that only agent tools (which use the NS separator per ConvertToPythonIdentifier in go/internal/utils/common.go) will generate clickable links, while regular tool calls will not.
When debugging agent behavior it is very useful to be able to drill down into the sub-agent activity and what tool calls they had - or even drill down into their sub-agent calls, and so on.
I find this feature very useful in agent development with sub-agents and sub-sub-agents.
I'm not super happy with this implementation, there are some hacks in here. It does basically work, though (at least it does in my main branch with all my customizations in it). I'd be happy to get some tips on a better way to achieve this.