From f510cae81514fbe4718131f1d32a6a44ce40762a Mon Sep 17 00:00:00 2001 From: Jose Armesto Date: Thu, 24 Sep 2026 18:23:01 +0200 Subject: [PATCH 1/3] fix(adk): flag the function response of a failed tool call with isError adk-go's MCP toolset turns a CallToolResult that carries isError into a Go error, and the flow renders every tool error as {"error": text}. The harness runtimes (harness/runtime/a2a) set isError beside a failed result, so a client that shows a step as failed on that flag sees a failed Go ADK tool as a successful step, with the error prose as its output. The on-tool-error callback now returns the same {"error": text} with "isError": true. The model reads the text it read before; the A2A function_response part carries the flag, as it does for the other runtimes. A confirmation request is a pause, not a failure, and keeps the plain shape; a rejected call did not run and is flagged. Signed-off-by: Jose Armesto --- go/adk/pkg/agent/agent.go | 29 +++++++++++++- go/adk/pkg/agent/tool_error_test.go | 62 +++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 2 deletions(-) create mode 100644 go/adk/pkg/agent/tool_error_test.go diff --git a/go/adk/pkg/agent/agent.go b/go/adk/pkg/agent/agent.go index c7f02b20d7..1d25273cbe 100644 --- a/go/adk/pkg/agent/agent.go +++ b/go/adk/pkg/agent/agent.go @@ -3,6 +3,7 @@ package agent import ( "context" "encoding/json" + "errors" "fmt" "os" "strings" @@ -516,7 +517,15 @@ func makeAfterToolCallback(logger *slog.Logger) llmagent.AfterToolCallback { } } -// makeOnToolErrorCallback returns an OnToolErrorCallback that logs tool errors. +// Keys of the function response of a tool call that failed: adk-go's own +// {"error": text} shape, and the flag kagent's clients read on every runtime. +const ( + toolResponseErrorKey = "error" + toolResponseIsErrorKey = "isError" +) + +// makeOnToolErrorCallback returns an OnToolErrorCallback that logs tool errors +// and marks the function response of a failed call with isError. func makeOnToolErrorCallback(logger *slog.Logger) llmagent.OnToolErrorCallback { return func(ctx agent.Context, t tool.Tool, args map[string]any, err error) (map[string]any, error) { logger.ErrorContext(ctx, "tool execution failed", "error", err, @@ -525,8 +534,24 @@ func makeOnToolErrorCallback(logger *slog.Logger) llmagent.OnToolErrorCallback { "session_id", ctx.SessionID(), "invocation_id", ctx.InvocationID(), ) - return nil, nil + return toolErrorResponse(err), nil + } +} + +// toolErrorResponse is the function response for a tool call that failed: the +// {"error": text} adk-go builds on its own, plus isError, the flag the harness +// runtimes set beside a failed result (harness/runtime/a2a) and clients read to +// show a step as failed. adk-go's MCP toolset collapses a CallToolResult with +// IsError into a Go error before it reaches the flow, so this callback is the +// one place that still knows the call failed. A confirmation request travels +// as an error too, but it is a pause, not a failure: it keeps the plain shape, +// which clients tell apart by its text. A nil map hands the error back to +// adk-go's default handling. +func toolErrorResponse(err error) map[string]any { + if err == nil || errors.Is(err, tool.ErrConfirmationRequired) { + return nil } + return map[string]any{toolResponseErrorKey: err.Error(), toolResponseIsErrorKey: true} } // mapKeys returns the top-level keys of a map for logging without exposing values. diff --git a/go/adk/pkg/agent/tool_error_test.go b/go/adk/pkg/agent/tool_error_test.go new file mode 100644 index 0000000000..5e8ed86309 --- /dev/null +++ b/go/adk/pkg/agent/tool_error_test.go @@ -0,0 +1,62 @@ +package agent + +import ( + "context" + "errors" + "fmt" + "log/slog" + "testing" + + "github.com/stretchr/testify/require" + adkagent "google.golang.org/adk/v2/agent" + "google.golang.org/adk/v2/tool" +) + +// toolErrorContext is the slice of agent.Context the on-tool-error callback +// reads for its log line. +type toolErrorContext struct { + adkagent.StrictContextMock +} + +func (toolErrorContext) FunctionCallID() string { return "call-1" } +func (toolErrorContext) SessionID() string { return "session-1" } +func (toolErrorContext) InvocationID() string { return "invocation-1" } + +type namedTool struct{ name string } + +func (t namedTool) Name() string { return t.name } +func (namedTool) Description() string { return "" } +func (namedTool) IsLongRunning() bool { return false } + +func newToolErrorContext() *toolErrorContext { + return &toolErrorContext{StrictContextMock: adkagent.NewStrictContextMock(context.Background())} +} + +func TestOnToolErrorCallback_FlagsAFailedCall(t *testing.T) { + callback := makeOnToolErrorCallback(slog.New(slog.DiscardHandler)) + // The error adk-go's MCP toolset returns for a CallToolResult with IsError set. + message := "Tool execution failed. Details: bad_data: invalid parameter \"query\": parse error" + err := errors.New(message) + + response, cbErr := callback(newToolErrorContext(), namedTool{name: "call_tool"}, nil, err) + + require.NoError(t, cbErr) + require.Equal(t, map[string]any{"error": message, "isError": true}, response) +} + +func TestOnToolErrorCallback_LeavesAConfirmationRequestToTheFlow(t *testing.T) { + callback := makeOnToolErrorCallback(slog.New(slog.DiscardHandler)) + err := fmt.Errorf("error tool %q %w", "call_tool", tool.ErrConfirmationRequired) + + response, cbErr := callback(newToolErrorContext(), namedTool{name: "call_tool"}, nil, err) + + require.NoError(t, cbErr) + require.Nil(t, response, "a confirmation request is a pause, not a failure: adk-go builds the plain response") +} + +func TestToolErrorResponse(t *testing.T) { + rejected := fmt.Errorf("error tool %q %w", "call_tool", tool.ErrConfirmationRejected) + require.Equal(t, map[string]any{"error": rejected.Error(), "isError": true}, toolErrorResponse(rejected), + "a rejected call did not run, so it is a failed call") + require.Nil(t, toolErrorResponse(nil)) +} From 191a05dd74572462bf15144e17e71bd7cc6c2561 Mon Sep 17 00:00:00 2001 From: Jose Armesto Date: Tue, 29 Sep 2026 17:13:14 +0200 Subject: [PATCH 2/3] refactor(adk): build the failed tool call's response in the callback Signed-off-by: Jose Armesto --- go/adk/pkg/agent/agent.go | 29 +++++------------------------ go/adk/pkg/agent/tool_error_test.go | 17 ++++++++++------- 2 files changed, 15 insertions(+), 31 deletions(-) diff --git a/go/adk/pkg/agent/agent.go b/go/adk/pkg/agent/agent.go index 1d25273cbe..9dfbd735c6 100644 --- a/go/adk/pkg/agent/agent.go +++ b/go/adk/pkg/agent/agent.go @@ -517,13 +517,6 @@ func makeAfterToolCallback(logger *slog.Logger) llmagent.AfterToolCallback { } } -// Keys of the function response of a tool call that failed: adk-go's own -// {"error": text} shape, and the flag kagent's clients read on every runtime. -const ( - toolResponseErrorKey = "error" - toolResponseIsErrorKey = "isError" -) - // makeOnToolErrorCallback returns an OnToolErrorCallback that logs tool errors // and marks the function response of a failed call with isError. func makeOnToolErrorCallback(logger *slog.Logger) llmagent.OnToolErrorCallback { @@ -534,24 +527,12 @@ func makeOnToolErrorCallback(logger *slog.Logger) llmagent.OnToolErrorCallback { "session_id", ctx.SessionID(), "invocation_id", ctx.InvocationID(), ) - return toolErrorResponse(err), nil - } -} - -// toolErrorResponse is the function response for a tool call that failed: the -// {"error": text} adk-go builds on its own, plus isError, the flag the harness -// runtimes set beside a failed result (harness/runtime/a2a) and clients read to -// show a step as failed. adk-go's MCP toolset collapses a CallToolResult with -// IsError into a Go error before it reaches the flow, so this callback is the -// one place that still knows the call failed. A confirmation request travels -// as an error too, but it is a pause, not a failure: it keeps the plain shape, -// which clients tell apart by its text. A nil map hands the error back to -// adk-go's default handling. -func toolErrorResponse(err error) map[string]any { - if err == nil || errors.Is(err, tool.ErrConfirmationRequired) { - return nil + // A confirmation request is a pause, not a failure; clients match its plain text. + if errors.Is(err, tool.ErrConfirmationRequired) { + return nil, nil + } + return map[string]any{"error": err.Error(), "isError": true}, nil } - return map[string]any{toolResponseErrorKey: err.Error(), toolResponseIsErrorKey: true} } // mapKeys returns the top-level keys of a map for logging without exposing values. diff --git a/go/adk/pkg/agent/tool_error_test.go b/go/adk/pkg/agent/tool_error_test.go index 5e8ed86309..f0421ccf86 100644 --- a/go/adk/pkg/agent/tool_error_test.go +++ b/go/adk/pkg/agent/tool_error_test.go @@ -44,6 +44,16 @@ func TestOnToolErrorCallback_FlagsAFailedCall(t *testing.T) { require.Equal(t, map[string]any{"error": message, "isError": true}, response) } +func TestOnToolErrorCallback_FlagsARejectedCall(t *testing.T) { + callback := makeOnToolErrorCallback(slog.New(slog.DiscardHandler)) + err := fmt.Errorf("error tool %q %w", "call_tool", tool.ErrConfirmationRejected) + + response, cbErr := callback(newToolErrorContext(), namedTool{name: "call_tool"}, nil, err) + + require.NoError(t, cbErr) + require.Equal(t, map[string]any{"error": err.Error(), "isError": true}, response, "a rejected call did not run") +} + func TestOnToolErrorCallback_LeavesAConfirmationRequestToTheFlow(t *testing.T) { callback := makeOnToolErrorCallback(slog.New(slog.DiscardHandler)) err := fmt.Errorf("error tool %q %w", "call_tool", tool.ErrConfirmationRequired) @@ -53,10 +63,3 @@ func TestOnToolErrorCallback_LeavesAConfirmationRequestToTheFlow(t *testing.T) { require.NoError(t, cbErr) require.Nil(t, response, "a confirmation request is a pause, not a failure: adk-go builds the plain response") } - -func TestToolErrorResponse(t *testing.T) { - rejected := fmt.Errorf("error tool %q %w", "call_tool", tool.ErrConfirmationRejected) - require.Equal(t, map[string]any{"error": rejected.Error(), "isError": true}, toolErrorResponse(rejected), - "a rejected call did not run, so it is a failed call") - require.Nil(t, toolErrorResponse(nil)) -} From c659131ade080c2714be749ead04f62eef39891f Mon Sep 17 00:00:00 2001 From: Jose Armesto Date: Mon, 19 Oct 2026 17:46:44 +0200 Subject: [PATCH 3/3] test(adk): check the flagged tool error through adk-go's flow The tests called the on-tool-error callback and read its return value. They did not show that adk-go puts that value into the function response, which is what a client reads. They now run an agent through adk-go's runner with the tool callbacks createGoogleADKAgent wires, and read the function responses the flow emits: a failed call is flagged, a confirmation request keeps the plain shape, and the rejection of that request does not run the tool and is flagged. Signed-off-by: Jose Armesto --- go/adk/pkg/agent/tool_error_test.go | 160 +++++++++++++++++++++------- 1 file changed, 122 insertions(+), 38 deletions(-) diff --git a/go/adk/pkg/agent/tool_error_test.go b/go/adk/pkg/agent/tool_error_test.go index f0421ccf86..2988c4e72c 100644 --- a/go/adk/pkg/agent/tool_error_test.go +++ b/go/adk/pkg/agent/tool_error_test.go @@ -3,63 +3,147 @@ package agent import ( "context" "errors" - "fmt" + "iter" "log/slog" + "sync" "testing" "github.com/stretchr/testify/require" adkagent "google.golang.org/adk/v2/agent" + "google.golang.org/adk/v2/agent/llmagent" + adkmodel "google.golang.org/adk/v2/model" + "google.golang.org/adk/v2/runner" + adksession "google.golang.org/adk/v2/session" "google.golang.org/adk/v2/tool" + "google.golang.org/adk/v2/tool/functiontool" + "google.golang.org/adk/v2/tool/toolconfirmation" + "google.golang.org/genai" ) -// toolErrorContext is the slice of agent.Context the on-tool-error callback -// reads for its log line. -type toolErrorContext struct { - adkagent.StrictContextMock -} - -func (toolErrorContext) FunctionCallID() string { return "call-1" } -func (toolErrorContext) SessionID() string { return "session-1" } -func (toolErrorContext) InvocationID() string { return "invocation-1" } +// The error adk-go's MCP toolset returns for a CallToolResult with IsError set. +const mcpToolError = "Tool execution failed. Details: bad_data: invalid parameter \"query\": parse error" -type namedTool struct{ name string } +type queryArgs struct { + Query string `json:"query"` +} -func (t namedTool) Name() string { return t.name } -func (namedTool) Description() string { return "" } -func (namedTool) IsLongRunning() bool { return false } +// toolCallingModel asks for one call of the tool on its first request and +// answers with text on every later one, so a turn ends once the function +// response is in the history. +type toolCallingModel struct { + mu sync.Mutex + requests int + tool string +} -func newToolErrorContext() *toolErrorContext { - return &toolErrorContext{StrictContextMock: adkagent.NewStrictContextMock(context.Background())} +func (m *toolCallingModel) Name() string { return "tool-calling" } + +func (m *toolCallingModel) GenerateContent(_ context.Context, _ *adkmodel.LLMRequest, _ bool) iter.Seq2[*adkmodel.LLMResponse, error] { + m.mu.Lock() + m.requests++ + first := m.requests == 1 + m.mu.Unlock() + return func(yield func(*adkmodel.LLMResponse, error) bool) { + if first { + yield(&adkmodel.LLMResponse{Content: &genai.Content{Role: genai.RoleModel, Parts: []*genai.Part{{ + FunctionCall: &genai.FunctionCall{ID: "call-1", Name: m.tool, Args: map[string]any{"query": "up"}}, + }}}}, nil) + return + } + yield(&adkmodel.LLMResponse{Content: genai.NewContentFromText("done", genai.RoleModel), FinishReason: genai.FinishReasonStop}, nil) + } } -func TestOnToolErrorCallback_FlagsAFailedCall(t *testing.T) { - callback := makeOnToolErrorCallback(slog.New(slog.DiscardHandler)) - // The error adk-go's MCP toolset returns for a CallToolResult with IsError set. - message := "Tool execution failed. Details: bad_data: invalid parameter \"query\": parse error" - err := errors.New(message) +// toolRunner runs an agent with the tool callbacks createGoogleADKAgent wires, +// through adk-go's flow, so the tests see the function responses it emits. +type toolRunner struct { + runner *runner.Runner + sessionID string +} - response, cbErr := callback(newToolErrorContext(), namedTool{name: "call_tool"}, nil, err) +func newToolRunner(t *testing.T, ctx context.Context, tools ...tool.Tool) *toolRunner { + t.Helper() + log := slog.New(slog.DiscardHandler) + a, err := llmagent.New(llmagent.Config{ + Name: "tools", + Model: &toolCallingModel{tool: tools[0].Name()}, + Tools: tools, + BeforeToolCallbacks: []llmagent.BeforeToolCallback{makeBeforeToolCallback(log)}, + AfterToolCallbacks: []llmagent.AfterToolCallback{makeAfterToolCallback(log)}, + OnToolErrorCallbacks: []llmagent.OnToolErrorCallback{makeOnToolErrorCallback(log)}, + }) + require.NoError(t, err) + sessions := adksession.InMemoryService() + r, err := runner.New(runner.Config{AppName: "test", Agent: a, SessionService: sessions}) + require.NoError(t, err) + sess, err := sessions.Create(ctx, &adksession.CreateRequest{AppName: "test", UserID: "user"}) + require.NoError(t, err) + return &toolRunner{runner: r, sessionID: sess.Session.ID()} +} - require.NoError(t, cbErr) - require.Equal(t, map[string]any{"error": message, "isError": true}, response) +// turn runs one turn and returns the function responses and function calls +// the flow emitted, in order. +func (r *toolRunner) turn(t *testing.T, ctx context.Context, content *genai.Content) (responses []*genai.FunctionResponse, calls []*genai.FunctionCall) { + t.Helper() + for event, err := range r.runner.Run(ctx, "user", r.sessionID, content, adkagent.RunConfig{}) { + require.NoError(t, err) + if event.Content == nil { + continue + } + for _, part := range event.Content.Parts { + if part.FunctionResponse != nil { + responses = append(responses, part.FunctionResponse) + } + if part.FunctionCall != nil { + calls = append(calls, part.FunctionCall) + } + } + } + return responses, calls } -func TestOnToolErrorCallback_FlagsARejectedCall(t *testing.T) { - callback := makeOnToolErrorCallback(slog.New(slog.DiscardHandler)) - err := fmt.Errorf("error tool %q %w", "call_tool", tool.ErrConfirmationRejected) +func TestFailedToolCallIsFlaggedInTheFunctionResponse(t *testing.T) { + t.Parallel() + ctx := t.Context() + failing, err := functiontool.New(functiontool.Config{Name: "query_prometheus"}, func(adkagent.Context, queryArgs) (map[string]any, error) { + return nil, errors.New(mcpToolError) + }) + require.NoError(t, err) - response, cbErr := callback(newToolErrorContext(), namedTool{name: "call_tool"}, nil, err) + responses, _ := newToolRunner(t, ctx, failing).turn(t, ctx, genai.NewContentFromText("is prometheus up?", genai.RoleUser)) - require.NoError(t, cbErr) - require.Equal(t, map[string]any{"error": err.Error(), "isError": true}, response, "a rejected call did not run") + require.Len(t, responses, 1) + require.Equal(t, "query_prometheus", responses[0].Name) + require.Equal(t, map[string]any{"error": mcpToolError, "isError": true}, responses[0].Response) } -func TestOnToolErrorCallback_LeavesAConfirmationRequestToTheFlow(t *testing.T) { - callback := makeOnToolErrorCallback(slog.New(slog.DiscardHandler)) - err := fmt.Errorf("error tool %q %w", "call_tool", tool.ErrConfirmationRequired) - - response, cbErr := callback(newToolErrorContext(), namedTool{name: "call_tool"}, nil, err) - - require.NoError(t, cbErr) - require.Nil(t, response, "a confirmation request is a pause, not a failure: adk-go builds the plain response") +func TestConfirmationRequestIsAPauseAndRejectionAFailure(t *testing.T) { + t.Parallel() + ctx := t.Context() + ran := false + guarded, err := functiontool.New(functiontool.Config{Name: "delete_pod", RequireConfirmation: true}, func(adkagent.Context, queryArgs) (map[string]any, error) { + ran = true + return map[string]any{"result": "deleted"}, nil + }) + require.NoError(t, err) + r := newToolRunner(t, ctx, guarded) + + responses, calls := r.turn(t, ctx, genai.NewContentFromText("delete the pod", genai.RoleUser)) + + require.Len(t, responses, 1, "the pending call answers with adk-go's plain response") + require.Contains(t, responses[0].Response, "error") + require.NotContains(t, responses[0].Response, "isError", "a confirmation request is a pause, not a failure") + require.Len(t, calls, 2, "the model's call, then the confirmation request") + require.Equal(t, toolconfirmation.FunctionCallName, calls[1].Name) + + // The user rejects the call: the tool does not run, and the call is flagged. + rejection := &genai.Content{Role: genai.RoleUser, Parts: []*genai.Part{{FunctionResponse: &genai.FunctionResponse{ + ID: calls[1].ID, Name: toolconfirmation.FunctionCallName, Response: map[string]any{"confirmed": false}, + }}}} + responses, _ = r.turn(t, ctx, rejection) + + require.False(t, ran, "a rejected call must not run") + require.Len(t, responses, 1) + require.Equal(t, "delete_pod", responses[0].Name) + require.Equal(t, map[string]any{"error": "error tool \"delete_pod\" call is rejected", "isError": true}, responses[0].Response) }