From e5383459d19f9b875447507827ee108dc657d6c4 Mon Sep 17 00:00:00 2001 From: GerardGao Date: Sun, 23 Aug 2026 14:01:23 +0800 Subject: [PATCH 1/2] tool/loadartifactstool: return error instead of panic when artifact service is not configured --- agent/common_context.go | 15 +++++++++-- tool/loadartifactstool/load_artifacts_tool.go | 6 ++++- .../load_artifacts_tool_test.go | 25 +++++++++++++++++++ 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/agent/common_context.go b/agent/common_context.go index 7bd19e3c9..154bcad2b 100644 --- a/agent/common_context.go +++ b/agent/common_context.go @@ -94,7 +94,7 @@ func NewCallbackContextWithArtifactTracking(ic InvocationContext, actions *sessi Context: ic, invocationContext: ic, actions: actions, - artifacts: &trackedArtifacts{Artifacts: ic.Artifacts(), actions: actions}, + artifacts: newTrackedArtifacts(ic.Artifacts(), actions), } // wrap the commonContext in order to log information about someone using tool-context methods on a callback context wrapper := &callbackContextWrapper{ @@ -132,7 +132,7 @@ func NewToolContext(ic InvocationContext, functionCallID string, actions *sessio res.actions = actions res.functionCallID = functionCallID res.toolConfirmation = confirmation - res.artifacts = &trackedArtifacts{Artifacts: ic.Artifacts(), actions: actions} + res.artifacts = newTrackedArtifacts(ic.Artifacts(), actions) wrapper := &toolContextWrapper{ context: &res, @@ -461,6 +461,17 @@ func (c *callbackContextState) All() iter.Seq2[string, any] { return c.ctx.invocationContext.Session().State().All() } +// newTrackedArtifacts wraps inner so that each successful Save is recorded +// into the supplied EventActions.ArtifactDelta. It returns nil when inner is +// nil so that "no artifact service configured" stays observable (a nil +// Artifacts) instead of panicking on the first promoted method call. +func newTrackedArtifacts(inner Artifacts, actions *session.EventActions) Artifacts { + if inner == nil { + return nil + } + return &trackedArtifacts{Artifacts: inner, actions: actions} +} + // trackedArtifacts wraps an Artifacts to record each successful Save into the // supplied EventActions.ArtifactDelta. type trackedArtifacts struct { diff --git a/tool/loadartifactstool/load_artifacts_tool.go b/tool/loadartifactstool/load_artifacts_tool.go index cfb0fa0c7..52b2910ae 100644 --- a/tool/loadartifactstool/load_artifacts_tool.go +++ b/tool/loadartifactstool/load_artifacts_tool.go @@ -128,7 +128,11 @@ func (t *artifactsTool) ProcessRequest(ctx agent.Context, req *model.LLMRequest) } func (t *artifactsTool) appendInitialInstructions(ctx agent.Context, req *model.LLMRequest) error { - resp, err := ctx.Artifacts().List(ctx) + artifacts := ctx.Artifacts() + if artifacts == nil { + return fmt.Errorf("load_artifacts tool requires an artifact service to be configured") + } + resp, err := artifacts.List(ctx) if err != nil { return fmt.Errorf("failed to list artifacts: %w", err) } diff --git a/tool/loadartifactstool/load_artifacts_tool_test.go b/tool/loadartifactstool/load_artifacts_tool_test.go index f60778e15..9f5ad1b07 100644 --- a/tool/loadartifactstool/load_artifacts_tool_test.go +++ b/tool/loadartifactstool/load_artifacts_tool_test.go @@ -339,6 +339,31 @@ func TestLoadArtifactsTool_ProcessRequest_Artifacts_OtherFunctionCall(t *testing } } +// TestLoadArtifactsTool_ProcessRequest_NilArtifacts verifies that ProcessRequest +// returns a descriptive error instead of panicking when no artifact service is +// configured (see https://github.com/google/adk-go/issues/283). +func TestLoadArtifactsTool_ProcessRequest_NilArtifacts(t *testing.T) { + loadArtifactsTool := loadartifactstool.New() + + // Construct a context with no artifact service configured. + invocationCtx := icontext.NewInvocationContext(t.Context(), icontext.InvocationContextParams{}) + tc := agent.NewToolContext(invocationCtx, "", nil, nil) + + requestProcessor, ok := loadArtifactsTool.(toolinternal.RequestProcessor) + if !ok { + t.Fatal("loadArtifactsTool does not implement RequestProcessor") + } + + llmRequest := &model.LLMRequest{} + err := requestProcessor.ProcessRequest(tc, llmRequest) + if err == nil { + t.Fatal("ProcessRequest should return an error when no artifact service is configured, but got nil") + } + if !strings.Contains(err.Error(), "artifact service") { + t.Errorf("error should mention the missing artifact service, got: %v", err) + } +} + func createToolContext(t *testing.T) agent.Context { t.Helper() From 455d4be559987c6223966f5f09eba4f8f8377e0e Mon Sep 17 00:00:00 2001 From: GerardGao Date: Thu, 27 Aug 2026 21:50:47 +0800 Subject: [PATCH 2/2] tool/loadartifactstool: hoist nil-artifact guard to ProcessRequest entry Per review: the guard lived in appendInitialInstructions, so the ctx.Artifacts() read in processLoadArtifactsFunctionCall was only protected by call ordering. Move the check to the top of ProcessRequest so both paths are covered in one place, and add a test that exercises the function-call path through the public entry. --- tool/loadartifactstool/load_artifacts_tool.go | 9 ++-- .../load_artifacts_tool_test.go | 44 +++++++++++++++++++ 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/tool/loadartifactstool/load_artifacts_tool.go b/tool/loadartifactstool/load_artifacts_tool.go index 52b2910ae..223f2b34d 100644 --- a/tool/loadartifactstool/load_artifacts_tool.go +++ b/tool/loadartifactstool/load_artifacts_tool.go @@ -118,6 +118,9 @@ func (t *artifactsTool) Run(ctx agent.Context, args any) (map[string]any, error) // ProcessRequest processes the LLM request. It packs the tool, appends initial // instructions, and processes any load artifacts function calls. func (t *artifactsTool) ProcessRequest(ctx agent.Context, req *model.LLMRequest) error { + if ctx.Artifacts() == nil { + return fmt.Errorf("load_artifacts tool requires an artifact service to be configured") + } if err := toolutils.PackTool(req, t); err != nil { return err } @@ -128,11 +131,7 @@ func (t *artifactsTool) ProcessRequest(ctx agent.Context, req *model.LLMRequest) } func (t *artifactsTool) appendInitialInstructions(ctx agent.Context, req *model.LLMRequest) error { - artifacts := ctx.Artifacts() - if artifacts == nil { - return fmt.Errorf("load_artifacts tool requires an artifact service to be configured") - } - resp, err := artifacts.List(ctx) + resp, err := ctx.Artifacts().List(ctx) if err != nil { return fmt.Errorf("failed to list artifacts: %w", err) } diff --git a/tool/loadartifactstool/load_artifacts_tool_test.go b/tool/loadartifactstool/load_artifacts_tool_test.go index 9f5ad1b07..22e98264c 100644 --- a/tool/loadartifactstool/load_artifacts_tool_test.go +++ b/tool/loadartifactstool/load_artifacts_tool_test.go @@ -364,6 +364,50 @@ func TestLoadArtifactsTool_ProcessRequest_NilArtifacts(t *testing.T) { } } +// TestLoadArtifactsTool_ProcessRequest_NilArtifacts_WithFunctionCall verifies +// that the nil-artifact guard in ProcessRequest also covers the +// processLoadArtifactsFunctionCall path, not only appendInitialInstructions: +// with a request whose last content is a load_artifacts function response, +// ProcessRequest must still return the descriptive error instead of panicking +// on the unguarded ctx.Artifacts() read in that path. +func TestLoadArtifactsTool_ProcessRequest_NilArtifacts_WithFunctionCall(t *testing.T) { + loadArtifactsTool := loadartifactstool.New() + + // Construct a context with no artifact service configured. + invocationCtx := icontext.NewInvocationContext(t.Context(), icontext.InvocationContextParams{}) + tc := agent.NewToolContext(invocationCtx, "", nil, nil) + + functionResponse := &genai.FunctionResponse{ + Name: "load_artifacts", + Response: map[string]any{ + "artifact_names": []any{"doc1.txt"}, + }, + } + llmRequest := &model.LLMRequest{ + Contents: []*genai.Content{ + { + Role: "model", + Parts: []*genai.Part{ + genai.NewPartFromFunctionResponse(functionResponse.Name, functionResponse.Response), + }, + }, + }, + } + + requestProcessor, ok := loadArtifactsTool.(toolinternal.RequestProcessor) + if !ok { + t.Fatal("loadArtifactsTool does not implement RequestProcessor") + } + + err := requestProcessor.ProcessRequest(tc, llmRequest) + if err == nil { + t.Fatal("ProcessRequest should return an error when no artifact service is configured, but got nil") + } + if !strings.Contains(err.Error(), "artifact service") { + t.Errorf("error should mention the missing artifact service, got: %v", err) + } +} + func createToolContext(t *testing.T) agent.Context { t.Helper()