diff --git a/docs/architecture/human-in-the-loop.md b/docs/architecture/human-in-the-loop.md index fe57071b6d..ddaa5207cd 100644 --- a/docs/architecture/human-in-the-loop.md +++ b/docs/architecture/human-in-the-loop.md @@ -101,6 +101,11 @@ The kagent Go harness maps the contract to ADK confirmations. Remote A2A tools i the Go and Python ADKs can retain a child continuation when a subagent requests input. +A rejected call is reported to the model as `Tool call was rejected by user.`, +followed by `Reason: ` when the decision carried a `rejection_reason`. +The Go and Python ADKs say the same, so a model can answer the question behind a +rejection and ask for the call again. + The current MCP tool-binding API does not expose per-tool approval policy, so users cannot configure generic MCP approval gating through `AgentTemplate` today. The extension remains valid for runtimes and tools that actually produce a pause, diff --git a/go/adk/pkg/a2a/hitl.go b/go/adk/pkg/a2a/hitl.go index 2e7f880127..22c47f4afd 100644 --- a/go/adk/pkg/a2a/hitl.go +++ b/go/adk/pkg/a2a/hitl.go @@ -617,7 +617,7 @@ func processDirectApproval(req *apia2a.ToolApprovalRequest, message *a2atype.Mes } var payload map[string]any if !approval.Approved && approval.RejectionReason != "" { - payload = map[string]any{"rejection_reason": approval.RejectionReason} + payload = map[string]any{apia2a.ToolConfirmationRejectionReasonKey: approval.RejectionReason} } parts = append(parts, buildConfirmationResponsePart(tool.ID, approval.Approved, payload)) delete(approvals, tool.ID) diff --git a/go/adk/pkg/agent/agent.go b/go/adk/pkg/agent/agent.go index 6b4edf21ae..585ba25e57 100644 --- a/go/adk/pkg/agent/agent.go +++ b/go/adk/pkg/agent/agent.go @@ -3,8 +3,10 @@ package agent import ( "context" "encoding/json" + "errors" "fmt" "os" + "strings" "log/slog" @@ -14,6 +16,7 @@ import ( adkoutputschema "github.com/kagent-dev/kagent/go/adk/pkg/outputschema" "github.com/kagent-dev/kagent/go/adk/pkg/sts" "github.com/kagent-dev/kagent/go/adk/pkg/tools" + apia2a "github.com/kagent-dev/kagent/go/api/a2a" "github.com/kagent-dev/kagent/go/api/adk" "github.com/kagent-dev/kagent/go/core/pkg/env" "github.com/kagent-dev/kagent/go/pkg/logging" @@ -26,6 +29,7 @@ import ( "google.golang.org/adk/v2/tool/preloadmemorytool" "google.golang.org/adk/v2/tool/skilltoolset" "google.golang.org/adk/v2/tool/skilltoolset/skill" + "google.golang.org/adk/v2/tool/toolconfirmation" "google.golang.org/genai" ) @@ -524,7 +528,11 @@ func makeAfterToolCallback(logger *slog.Logger) llmagent.AfterToolCallback { } } -// makeOnToolErrorCallback returns an OnToolErrorCallback that logs tool errors. +// rejectedToolMessage matches the Python runtime (kagent/adk/_approval.py). +const rejectedToolMessage = "Tool call was rejected by user." + +// 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, @@ -533,8 +541,30 @@ func makeOnToolErrorCallback(logger *slog.Logger) llmagent.OnToolErrorCallback { "session_id", ctx.SessionID(), "invocation_id", ctx.InvocationID(), ) - return nil, nil + // A confirmation request is a pause, not a failure; clients match its plain text. + if errors.Is(err, tool.ErrConfirmationRequired) { + return nil, nil + } + message := err.Error() + if errors.Is(err, tool.ErrConfirmationRejected) { + message = rejectionMessage(ctx.ToolConfirmation()) + } + return map[string]any{"error": message, "isError": true}, nil + } +} + +// rejectionMessage adds the reason the person gave, if any. A confirmation +// that stands for a child agent's decisions carries no reason of its own. +func rejectionMessage(confirmation *toolconfirmation.ToolConfirmation) string { + var payload map[string]any + if confirmation != nil { + payload, _ = confirmation.Payload.(map[string]any) + } + reason, _ := payload[apia2a.ToolConfirmationRejectionReasonKey].(string) + if reason = strings.TrimSpace(reason); reason == "" { + return rejectedToolMessage } + return rejectedToolMessage + " Reason: " + reason } // mapKeys returns the top-level keys of a map for logging without exposing values. diff --git a/go/adk/pkg/agent/rejection_resume_test.go b/go/adk/pkg/agent/rejection_resume_test.go new file mode 100644 index 0000000000..6760c59276 --- /dev/null +++ b/go/adk/pkg/agent/rejection_resume_test.go @@ -0,0 +1,142 @@ +package agent + +import ( + "context" + "iter" + "log/slog" + "sync" + "testing" + + a2atype "github.com/a2aproject/a2a-go/v2/a2a" + "github.com/a2aproject/a2a-go/v2/a2asrv" + kagenta2a "github.com/kagent-dev/kagent/go/adk/pkg/a2a" + apia2a "github.com/kagent-dev/kagent/go/api/a2a" + "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/genai" +) + +type deleteFileArgs struct { + Path string `json:"path"` +} + +// deleteCallModel calls delete_file on its first request and answers with text +// after that, keeping every request it sees. +type deleteCallModel struct { + mu sync.Mutex + requests []*adkmodel.LLMRequest +} + +func (m *deleteCallModel) Name() string { return "delete-call" } + +func (m *deleteCallModel) GenerateContent(_ context.Context, req *adkmodel.LLMRequest, _ bool) iter.Seq2[*adkmodel.LLMResponse, error] { + m.mu.Lock() + m.requests = append(m.requests, req) + first := len(m.requests) == 1 + m.mu.Unlock() + return func(yield func(*adkmodel.LLMResponse, error) bool) { + content := genai.NewContentFromText("done", genai.RoleModel) + if first { + call := genai.NewPartFromFunctionCall("delete_file", map[string]any{"path": "/tmp/x"}) + call.FunctionCall.ID = "delete-call" + content = &genai.Content{Role: genai.RoleModel, Parts: []*genai.Part{call}} + } + yield(&adkmodel.LLMResponse{Content: content, FinishReason: genai.FinishReasonStop}, nil) + } +} + +func (m *deleteCallModel) lastRequest() *adkmodel.LLMRequest { + m.mu.Lock() + defer m.mu.Unlock() + return m.requests[len(m.requests)-1] +} + +// The reason a person gives for rejecting a call crosses the A2A decision, the +// adk-go resume and the on-tool-error callback, and reaches the model as the +// response of the rejected call. +func TestRejectionReasonReachesTheModelOnResume(t *testing.T) { + const appName, taskID, contextID = "reject-app", "reject-task", "reject-context" + deleteFile, err := functiontool.New( + functiontool.Config{Name: "delete_file", Description: "Deletes a file.", RequireConfirmation: true}, + func(adkagent.Context, deleteFileArgs) (map[string]any, error) { + t.Error("a rejected call must not run") + return nil, nil + }, + ) + require.NoError(t, err) + llm := &deleteCallModel{} + logger := slog.New(slog.DiscardHandler) + root, err := llmagent.New(llmagent.Config{ + Name: "reject-agent", + Model: llm, + Tools: []tool.Tool{deleteFile}, + OnToolErrorCallbacks: []llmagent.OnToolErrorCallback{makeOnToolErrorCallback(logger)}, + }) + require.NoError(t, err) + executor, err := kagenta2a.NewKAgentExecutor(kagenta2a.KAgentExecutorConfig{ + AppName: appName, SessionService: adksession.InMemoryService(), Logger: logger, + RunnerConfig: runner.Config{AppName: appName, Agent: root}, + }) + require.NoError(t, err) + ctx, callCtx := a2asrv.NewCallContext(context.Background(), a2asrv.NewServiceParams(map[string][]string{ + a2atype.SvcParamExtensions: {kagenta2a.HITLExtensionURI}, + })) + _, _, err = kagenta2a.HITLActivationInterceptor().Before(ctx, callCtx, &a2asrv.Request{}) + require.NoError(t, err) + + first := &a2asrv.ExecutorContext{ + TaskID: taskID, ContextID: contextID, + Message: a2atype.NewMessage(a2atype.MessageRoleUser, a2atype.NewTextPart("delete /tmp/x")), + } + var pause *a2atype.TaskStatusUpdateEvent + for event, err := range executor.Execute(ctx, first) { + require.NoError(t, err) + if update, ok := event.(*a2atype.TaskStatusUpdateEvent); ok && update.Status.State == a2atype.TaskStateInputRequired { + pause = update + } + } + require.NotNil(t, pause, "the call waits for a decision") + approval := kagenta2a.GetToolApprovalRequest(pause.Status.Message) + require.NotNil(t, approval) + require.Len(t, approval.Tools, 1) + + decision := kagenta2a.AttachHitlExtension( + a2atype.NewMessage(a2atype.MessageRoleUser, a2atype.NewTextPart("Decision")), + &apia2a.ToolApprovalResponse{ + Type: kagenta2a.HITLTypeToolApprovalResponse, + Approvals: []apia2a.ToolApproval{{ + ID: approval.Tools[0].ID, Approved: false, RejectionReason: "What does this tool do?", + }}, + }, + ) + decision.TaskID, decision.ContextID = taskID, contextID + resume := &a2asrv.ExecutorContext{ + TaskID: taskID, ContextID: contextID, Message: decision, + StoredTask: &a2atype.Task{ + ID: taskID, ContextID: contextID, Status: pause.Status, History: []*a2atype.Message{first.Message, decision}, + }, + } + for _, err := range executor.Execute(ctx, resume) { + require.NoError(t, err) + } + + var rejected *genai.FunctionResponse + for _, content := range llm.lastRequest().Contents { + for _, part := range content.Parts { + if part.FunctionResponse != nil && part.FunctionResponse.ID == "delete-call" { + rejected = part.FunctionResponse + } + } + } + require.NotNil(t, rejected, "the model sees the response of the rejected call") + require.Equal(t, map[string]any{ + "error": "Tool call was rejected by user. Reason: What does this tool do?", + "isError": true, + }, rejected.Response) +} 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..8b626ebd0f --- /dev/null +++ b/go/adk/pkg/agent/tool_error_test.go @@ -0,0 +1,94 @@ +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" + "google.golang.org/adk/v2/tool/toolconfirmation" +) + +// toolErrorContext is the slice of agent.Context the on-tool-error callback +// reads: the fields of its log line, and the turn's confirmation. +type toolErrorContext struct { + adkagent.StrictContextMock + confirmation *toolconfirmation.ToolConfirmation +} + +func (toolErrorContext) FunctionCallID() string { return "call-1" } +func (toolErrorContext) SessionID() string { return "session-1" } +func (toolErrorContext) InvocationID() string { return "invocation-1" } + +func (c *toolErrorContext) ToolConfirmation() *toolconfirmation.ToolConfirmation { + return c.confirmation +} + +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(confirmation *toolconfirmation.ToolConfirmation) *toolErrorContext { + return &toolErrorContext{ + StrictContextMock: adkagent.NewStrictContextMock(context.Background()), + confirmation: confirmation, + } +} + +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(nil), namedTool{name: "call_tool"}, nil, err) + + require.NoError(t, cbErr) + require.Equal(t, map[string]any{"error": message, "isError": true}, response) +} + +func TestOnToolErrorCallback_TellsTheModelWhyAPersonRejectedACall(t *testing.T) { + callback := makeOnToolErrorCallback(slog.New(slog.DiscardHandler)) + err := fmt.Errorf("error tool %q %w", "call_tool", tool.ErrConfirmationRejected) + for _, tc := range []struct { + name string + payload map[string]any + want string + }{{ + name: "the reason the person gave", + payload: map[string]any{"rejection_reason": "What does this tool do?"}, + want: "Tool call was rejected by user. Reason: What does this tool do?", + }, { + name: "no reason given", + want: "Tool call was rejected by user.", + }, { + name: "a nested confirmation", + payload: map[string]any{"subagent_name": "child", "task_id": "child-task"}, + want: "Tool call was rejected by user.", + }} { + t.Run(tc.name, func(t *testing.T) { + ctx := newToolErrorContext(&toolconfirmation.ToolConfirmation{Payload: tc.payload}) + + response, cbErr := callback(ctx, namedTool{name: "call_tool"}, nil, err) + + require.NoError(t, cbErr) + require.Equal(t, map[string]any{"error": tc.want, "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(nil), 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") +} diff --git a/go/api/a2a/hitl.go b/go/api/a2a/hitl.go index 5e10a46d20..bd1157376f 100644 --- a/go/api/a2a/hitl.go +++ b/go/api/a2a/hitl.go @@ -56,6 +56,11 @@ type ToolApproval struct { RejectionReason string `json:"rejection_reason,omitempty"` } +// ToolConfirmationRejectionReasonKey is the key a harness adapter carries a +// rejected decision's reason under in the native confirmation payload, so that +// the runtime resuming the turn can tell the model why the person said no. +const ToolConfirmationRejectionReasonKey = "rejection_reason" + // ToolApprovalResponse is the public response payload for tool approvals. type ToolApprovalResponse struct { Type string `json:"type"` diff --git a/ui/src/api/chat/a2aGrpcChatClient.test.ts b/ui/src/api/chat/a2aGrpcChatClient.test.ts index ee163f406c..26f4de989b 100644 --- a/ui/src/api/chat/a2aGrpcChatClient.test.ts +++ b/ui/src/api/chat/a2aGrpcChatClient.test.ts @@ -1099,6 +1099,36 @@ describe("A2AGrpcChatClient.history", () => { ]); }); + it("renders an unpaired rejection carrying the person's reason as not run", async () => { + serveTasks([ + { + id: "task-1", + contextId: CONVERSATION.id, + status: { state: TaskState.COMPLETED, timestamp: { seconds: 1767225600n } }, + history: [], + artifacts: [ + { + artifactId: "rejected", + parts: [ + data({ + name: "k8s_get_resources", + response: { + error: "Tool call was rejected by user. Reason: What does this tool do?", + isError: true, + }, + }), + ], + }, + ], + }, + ]); + + const { messages } = await new A2AGrpcChatClient().history(CONVERSATION); + expect(messages[0].parts).toEqual([ + expect.objectContaining({ kind: "data", dataKind: "tool_not_run" }), + ]); + }); + it("hides a rejected FunctionResponse when the structured decision records it", async () => { serveTasks([ { diff --git a/ui/src/api/chat/a2aGrpcChatClient.ts b/ui/src/api/chat/a2aGrpcChatClient.ts index d4f239af32..747fe7edfd 100644 --- a/ui/src/api/chat/a2aGrpcChatClient.ts +++ b/ui/src/api/chat/a2aGrpcChatClient.ts @@ -282,8 +282,8 @@ type ControlFlowResult = "confirmation_required" | "rejected"; * * ADK currently reports confirmation as an errored FunctionResponse. Those * responses are instructions to the runtime, not failed tool executions. Match - * only the two complete compatibility strings observed on the wire; a broad - * substring check could hide a real tool error that merely discusses approval. + * only the complete strings observed on the wire; a broad substring check could + * hide a real tool error that merely discusses approval. */ function visibleParts( parts: readonly ChatPart[], @@ -331,9 +331,17 @@ function controlFlowResult(part: ChatDataPart): ControlFlowResult | undefined { if (confirmation && (!name || confirmation[1] === name)) return "confirmation_required"; const rejected = /^error tool "([^"]+)" call is rejected$/.exec(error); if (rejected && (!name || rejected[1] === name)) return "rejected"; + if (REJECTED_BY_USER.test(error)) return "rejected"; return undefined; } +/** + * What a runtime says when a person rejected the call, and the reason they gave + * when they gave one. It names the person rather than the tool, so there is no + * tool name in it to correlate. + */ +const REJECTED_BY_USER = /^Tool call was rejected by user\.(?: Reason: [\s\S]+)?$/; + /** The prose of a set of parts, for comparing a reply against the artifact repeating it. */ function textOf(parts: readonly ChatPart[]): string { return parts