From cf02aeaf41e2626044ef75df76d247f9712351ed Mon Sep 17 00:00:00 2001 From: Evan Rauner Date: Tue, 18 Aug 2026 14:52:31 -0500 Subject: [PATCH] fix(adk): validate ask_user questions Signed-off-by: Evan Rauner --- go/adk/pkg/tools/ask_user.go | 16 +- go/adk/pkg/tools/ask_user_test.go | 139 ++++++++++++++++++ .../src/kagent/adk/tools/ask_user_tool.py | 15 +- .../tests/unittests/test_ask_user_tool.py | 93 ++++++++++++ 4 files changed, 254 insertions(+), 9 deletions(-) create mode 100644 go/adk/pkg/tools/ask_user_test.go create mode 100644 python/packages/kagent-adk/tests/unittests/test_ask_user_tool.py diff --git a/go/adk/pkg/tools/ask_user.go b/go/adk/pkg/tools/ask_user.go index 9e9258bb6..f288d7a9a 100644 --- a/go/adk/pkg/tools/ask_user.go +++ b/go/adk/pkg/tools/ask_user.go @@ -20,8 +20,8 @@ type askUserInput struct { Questions []askUserQuestion `json:"questions"` } -const askUserDescription = "Ask the user one or more questions and wait for their answers " + - "before continuing. Use this when you need clarifying information, " + +const askUserDescription = "Ask the user at least one question and wait for their answers before continuing. " + + "Every question must include non-empty text. Use this when you need clarifying information, " + "preferences, or explicit confirmation from the user." // NewAskUserTool creates the ask_user tool using functiontool.New. @@ -41,6 +41,15 @@ func NewAskUserTool() (tool.Tool, error) { Name: "ask_user", Description: askUserDescription, }, func(ctx adkagent.Context, in askUserInput) (map[string]any, error) { + if len(in.Questions) == 0 { + return nil, fmt.Errorf("ask_user: at least one question is required") + } + for i, q := range in.Questions { + if strings.TrimSpace(q.Question) == "" { + return nil, fmt.Errorf("ask_user: question %d must contain non-whitespace text", i+1) + } + } + if ctx.ToolConfirmation() == nil { // Phase 1 — pause execution and ask the user. var sb strings.Builder @@ -51,9 +60,6 @@ func NewAskUserTool() (tool.Tool, error) { sb.WriteString(q.Question) } hint := sb.String() - if hint == "" { - hint = "Questions for the user." - } // Build questions slice for the pending response. questionsSlice := make([]map[string]any, 0, len(in.Questions)) diff --git a/go/adk/pkg/tools/ask_user_test.go b/go/adk/pkg/tools/ask_user_test.go new file mode 100644 index 000000000..b42445e7d --- /dev/null +++ b/go/adk/pkg/tools/ask_user_test.go @@ -0,0 +1,139 @@ +package tools + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + adkagent "google.golang.org/adk/v2/agent" + "google.golang.org/adk/v2/tool/toolconfirmation" +) + +type askUserConfirmationRequest struct { + hint string + payload any +} + +type askUserTestContext struct { + adkagent.Context + confirmation *toolconfirmation.ToolConfirmation + requests []askUserConfirmationRequest +} + +func (c *askUserTestContext) ToolConfirmation() *toolconfirmation.ToolConfirmation { + return c.confirmation +} + +func (c *askUserTestContext) RequestConfirmation(hint string, payload any) error { + c.requests = append(c.requests, askUserConfirmationRequest{hint: hint, payload: payload}) + return nil +} + +func runAskUserTool( + t *testing.T, + ctx adkagent.Context, + args map[string]any, +) (map[string]any, error) { + t.Helper() + + askUserTool, err := NewAskUserTool() + require.NoError(t, err) + runner, ok := askUserTool.(interface { + Run(adkagent.Context, any) (map[string]any, error) + }) + require.True(t, ok, "ask_user tool %T does not implement Run", askUserTool) + return runner.Run(ctx, args) +} + +func TestAskUserRejectsInvalidQuestionsWithoutRequestingConfirmation(t *testing.T) { + tests := []struct { + name string + args map[string]any + wantErr string + }{ + { + name: "empty list", + args: map[string]any{"questions": []any{}}, + wantErr: "ask_user: at least one question is required", + }, + { + name: "empty first question", + args: map[string]any{"questions": []any{map[string]any{"question": ""}}}, + wantErr: "ask_user: question 1 must contain non-whitespace text", + }, + { + name: "spaces-only first question", + args: map[string]any{"questions": []any{map[string]any{"question": " "}}}, + wantErr: "ask_user: question 1 must contain non-whitespace text", + }, + { + name: "tab-newline-only first question", + args: map[string]any{"questions": []any{map[string]any{"question": "\t\n"}}}, + wantErr: "ask_user: question 1 must contain non-whitespace text", + }, + { + name: "blank second question", + args: map[string]any{"questions": []any{ + map[string]any{"question": "Which environment?"}, + map[string]any{"question": " \t\n"}, + }}, + wantErr: "ask_user: question 2 must contain non-whitespace text", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := &askUserTestContext{} + + _, err := runAskUserTool(t, ctx, tt.args) + + assert.EqualError(t, err, tt.wantErr) + assert.Empty(t, ctx.requests) + }) + } +} + +func TestAskUserValidQuestionRequestsConfirmation(t *testing.T) { + ctx := &askUserTestContext{} + question := " Which environment? " + + result, err := runAskUserTool(t, ctx, map[string]any{ + "questions": []any{map[string]any{ + "question": question, + "choices": []any{"prod", "staging"}, + "multiple": true, + }}, + }) + + require.NoError(t, err) + assert.Equal(t, []askUserConfirmationRequest{{hint: question, payload: nil}}, ctx.requests) + assert.Equal(t, map[string]any{ + "status": "pending", + "questions": []any{map[string]any{ + "question": question, + "choices": []any{"prod", "staging"}, + "multiple": true, + }}, + }, result) +} + +func TestAskUserValidConfirmedQuestionReturnsAnswer(t *testing.T) { + ctx := &askUserTestContext{ + confirmation: &toolconfirmation.ToolConfirmation{ + Confirmed: true, + Payload: map[string]any{ + "answers": []any{map[string]any{"answer": "prod"}}, + }, + }, + } + + result, err := runAskUserTool(t, ctx, map[string]any{ + "questions": []any{map[string]any{"question": " Which environment? "}}, + }) + + require.NoError(t, err) + assert.Empty(t, ctx.requests) + resultJSON, ok := result["result"].(string) + require.True(t, ok) + assert.JSONEq(t, `[{"answer":"prod","question":" Which environment? "}]`, resultJSON) +} diff --git a/python/packages/kagent-adk/src/kagent/adk/tools/ask_user_tool.py b/python/packages/kagent-adk/src/kagent/adk/tools/ask_user_tool.py index caaa90ef9..f71dc01d7 100644 --- a/python/packages/kagent-adk/src/kagent/adk/tools/ask_user_tool.py +++ b/python/packages/kagent-adk/src/kagent/adk/tools/ask_user_tool.py @@ -57,8 +57,8 @@ def __init__(self) -> None: super().__init__( name="ask_user", description=( - "Ask the user one or more questions and wait for their answers " - "before continuing. Use this when you need clarifying information, " + "Ask the user at least one question and wait for their answers before continuing. " + "Every question must include non-empty text. Use this when you need clarifying information, " "preferences, or explicit confirmation from the user." ), ) @@ -88,10 +88,17 @@ async def run_async( ) -> Any: questions: list[dict] = args.get("questions", []) + if not questions: + raise ValueError("ask_user: at least one question is required") + for index, question in enumerate(questions, start=1): + question_text = question.get("question") + if not isinstance(question_text, str) or not question_text.strip(): + raise ValueError(f"ask_user: question {index} must contain non-whitespace text") + if tool_context.tool_confirmation is None: # First invocation — pause execution and ask the user. - summary = "; ".join(q.get("question", "") for q in questions if q.get("question")) - tool_context.request_confirmation(hint=summary or "Questions for the user.") + summary = "; ".join(q["question"] for q in questions) + tool_context.request_confirmation(hint=summary) logger.debug("ask_user: requesting confirmation with %d question(s)", len(questions)) return {"status": "pending", "questions": questions} diff --git a/python/packages/kagent-adk/tests/unittests/test_ask_user_tool.py b/python/packages/kagent-adk/tests/unittests/test_ask_user_tool.py new file mode 100644 index 000000000..15b507895 --- /dev/null +++ b/python/packages/kagent-adk/tests/unittests/test_ask_user_tool.py @@ -0,0 +1,93 @@ +import json +from unittest.mock import Mock + +import pytest +from google.adk.tools.tool_confirmation import ToolConfirmation +from google.adk.tools.tool_context import ToolContext + +from kagent.adk.tools.ask_user_tool import AskUserTool + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("args", "expected_error"), + [ + ( + {"questions": []}, + "ask_user: at least one question is required", + ), + ( + {"questions": [{"question": ""}]}, + "ask_user: question 1 must contain non-whitespace text", + ), + ( + {"questions": [{"question": " "}]}, + "ask_user: question 1 must contain non-whitespace text", + ), + ( + {"questions": [{"question": "\t\n"}]}, + "ask_user: question 1 must contain non-whitespace text", + ), + ( + { + "questions": [ + {"question": "Which environment?"}, + {"question": " \t\n"}, + ] + }, + "ask_user: question 2 must contain non-whitespace text", + ), + ], +) +async def test_rejects_invalid_questions_without_requesting_confirmation(args, expected_error): + context = Mock(spec=ToolContext) + context.tool_confirmation = None + + with pytest.raises(ValueError) as exc_info: + await AskUserTool().run_async(args=args, tool_context=context) + + assert str(exc_info.value) == expected_error + context.request_confirmation.assert_not_called() + + +@pytest.mark.asyncio +async def test_valid_question_requests_confirmation_without_rewriting_text(): + context = Mock(spec=ToolContext) + context.tool_confirmation = None + questions = [ + { + "question": " Which environment? ", + "choices": ["prod", "staging"], + "multiple": True, + } + ] + + result = await AskUserTool().run_async( + args={"questions": questions}, + tool_context=context, + ) + + assert result == {"status": "pending", "questions": questions} + context.request_confirmation.assert_called_once_with(hint=" Which environment? ") + + +@pytest.mark.asyncio +async def test_valid_confirmed_question_returns_answer_without_new_confirmation(): + context = Mock(spec=ToolContext) + context.tool_confirmation = ToolConfirmation( + confirmed=True, + payload={"answers": [{"answer": "prod"}]}, + ) + + result = await AskUserTool().run_async( + args={"questions": [{"question": " Which environment? "}]}, + tool_context=context, + ) + + assert json.loads(result) == [ + { + "question": " Which environment? ", + "answer": "prod", + } + ] + context.request_confirmation.assert_not_called()