load_artifacts: a failed artifact load aborts the whole run instead of going back to the model

Describe the Bug:

When the model calls load_artifacts with a name that can't be loaded (an artifact that doesn't exist, or a name with a / in it), the run ends with an error and the model never gets a chance to correct itself.

The reason is where the load happens. Run only echoes the names back, and the actual load happens on the next step, inside ProcessRequest (tool/loadartifactstool/load_artifacts_tool.go, processLoadArtifactsFunctionCall → loadIndividualArtifact). If the load fails, ProcessRequest returns the error, and toolPreprocess in internal/llminternal/base_flow.go (around line 953 on main) returns it straight up, so the flow stops before the next model call.

Compare that with any other tool: if Run fails, the flow turns the error into {"error": "..."} as the function response and the model sees it on the next call. For load_artifacts the mistake is just as recoverable (wrong name, a repo path instead of an artifact name), but the user gets a failed turn instead.

I hit this in production in an agent that also has a GitHub MCP. The model sometimes called load_artifacts with a repo path like src/foo.go instead of using the MCP's get_file_contents, and every time the user got a 500. I worked around it on my side by catching the error, but I think the default behaviour should be fixed here.

Steps to Reproduce:

Clone google/adk-go at main (I used 900e2fa, 15 Oct 2026).
Save the program below as cmd/repro_loadartifacts/main.go. It uses a fake model that calls load_artifacts once and, if it ever got the error back, would answer in text.
go run ./cmd/repro_loadartifacts

package main

import (
	"context"
	"fmt"
	"iter"

	"google.golang.org/genai"

	"google.golang.org/adk/v2/agent"
	"google.golang.org/adk/v2/agent/llmagent"
	"google.golang.org/adk/v2/artifact"
	"google.golang.org/adk/v2/model"
	"google.golang.org/adk/v2/runner"
	"google.golang.org/adk/v2/session"
	"google.golang.org/adk/v2/tool"
	"google.golang.org/adk/v2/tool/loadartifactstool"
)

type fakeModel struct{ calls int }

func (m *fakeModel) Name() string { return "fake" }

func (m *fakeModel) GenerateContent(ctx context.Context, req *model.LLMRequest, stream bool) iter.Seq2[*model.LLMResponse, error] {
	return func(yield func(*model.LLMResponse, error) bool) {
		m.calls++
		if m.calls == 1 {
			yield(&model.LLMResponse{Content: &genai.Content{
				Role: genai.RoleModel,
				Parts: []*genai.Part{genai.NewPartFromFunctionCall("load_artifacts",
					map[string]any{"artifact_names": []any{"src/main.go"}})},
			}}, nil)
			return
		}
		yield(&model.LLMResponse{Content: genai.NewContentFromText("Sorry, that artifact does not exist.", genai.RoleModel)}, nil)
	}
}

func main() {
	ctx := context.Background()
	m := &fakeModel{}
	a, err := llmagent.New(llmagent.Config{
		Name:  "repro",
		Model: m,
		Tools: []tool.Tool{loadartifactstool.New()},
	})
	if err != nil {
		panic(err)
	}
	ss := session.InMemoryService()
	r, err := runner.New(runner.Config{
		AppName:         "app",
		Agent:           a,
		SessionService:  ss,
		ArtifactService: artifact.InMemoryService(),
	})
	if err != nil {
		panic(err)
	}
	s, err := ss.Create(ctx, &session.CreateRequest{AppName: "app", UserID: "u"})
	if err != nil {
		panic(err)
	}
	for ev, err := range r.Run(ctx, "u", s.Session.ID(), genai.NewContentFromText("show me src/main.go", genai.RoleUser), agent.RunConfig{}) {
		if err != nil {
			fmt.Printf("run error: %v\n", err)
			break
		}
		for _, p := range ev.Content.Parts {
			switch {
			case p.FunctionCall != nil:
				fmt.Printf("event: function_call %s %v\n", p.FunctionCall.Name, p.FunctionCall.Args)
			case p.FunctionResponse != nil:
				fmt.Printf("event: function_response %s %v\n", p.FunctionResponse.Name, p.FunctionResponse.Response)
			case p.Text != "":
				fmt.Printf("event: text %q\n", p.Text)
			}
		}
	}
	fmt.Printf("model calls: %d\n", m.calls)
}

Expected Behavior:

The load failure reaches the model as a function result, the same way a Run error does, so the model gets a second call and can answer or try another tool:

event: function_call load_artifacts map[artifact_names:[src/main.go]]
event: function_response load_artifacts map[artifact_names:[src/main.go]]
event: text "Sorry, that artifact does not exist."
model calls: 2

Observed Behavior:

The run ends with an error and the model is called only once:

event: function_call load_artifacts map[artifact_names:[src/main.go]]
event: function_response load_artifacts map[artifact_names:[src/main.go]]
run error: failed to load artifact src/main.go: request validation failed: invalid name: filename cannot contain path separators
model calls: 1

It's the same with a plain name that doesn't exist (missing.txt):

run error: failed to load artifact missing.txt: artifact not found: file does not exist
model calls: 1

Environment Details:

ADK Library Version: main at 900e2fa (v2). The same code path is on v1.9.0 (ProcessRequest → processLoadArtifactsFunctionCall, and toolPreprocess returns the error). I first hit it on v1.5.0.
OS: macOS (Darwin 25.5)
Go Version: go1.26.6

Model Information:

The repro uses a fake model.LLM, so no model is involved. In production it was a Claude model behind a custom model.LLM.

Regression:

No, as far as I can tell it has always worked this way.

Additional context:

TestLoadArtifactsTool_ProcessRequest_LoadError (added in #1574) checks that ProcessRequest returns an error for a missing artifact. I read that test as pinning the error message (no double wrapping), not as a decision that the run should stop, so I don't think it argues against this change. But if aborting is intended, I think it should at least be documented on the tool.

Some ideas for the fix, any of these would work for me:

When a load fails in processLoadArtifactsFunctionCall, append a content part telling the model which names couldn't be loaded and why, keep the ones that did load, and return nil.
Or check the names in Run (artifact exists, valid name) and return the error from there, so it follows the normal tool-error path.

Related, and minor: the tool description ("Loads the artifacts and adds them to the session") doesn't say that artifacts are files saved in the artifact service, not repo or filesystem paths. In my case a clearer description cut most of the wrong calls.

I tried both of the fixes you suggest and each one collides with a test that currently asserts the present shape — so this needs a decision about which contract to change, which is why I am writing it up rather than opening a PR against my own guess.

Option 2, checking the names in Run, breaks TestLoadArtifactsTool_Run. Its basic string slice and any slice with strings subtests pass file1/file2 and fileA/fileB to Run without saving any of them, and assert no error. That test says Run is a pure echo whose job is argument parsing, and that nothing is expected to exist at that point. Making Run validate means either weakening a test unrelated to this bug, or accepting that Run's contract has changed. I had it working — the error surfaces as an ordinary tool error and the existing ProcessRequest test stays green — but it only passes with those two subtests rewritten to save their artifacts first, and I am not going to quietly rewrite a test to fit my implementation.

Option 1, making the load failure non-fatal in ProcessRequest, breaks TestLoadArtifactsTool_ProcessRequest_LoadError, whose first assertion is ProcessRequest should return an error when an artifact cannot be loaded. That one is more arguable than it looks, and the useful thing I can add is what #1574 actually was.

#1574 is titled "stop double-wrapping the artifact load error", and its body is about loadIndividualArtifact already wrapping the service error with the artifact name, so ProcessRequest wrapped it a second time. That test's other two assertions — the failure is named exactly once, and errors.Is(err, fs.ErrNotExist) holds — are precisely that concern. So I agree with your reading that the test is pinning the message, not endorsing the abort. But the first assertion is an explicit statement that ProcessRequest returns an error, and it is the only place in the repository that says so, so changing it is a contract change rather than a test that happens to fail.

What I could not settle from the code alone: with option 1, ["doc1.txt", "missing.txt"] should presumably load doc1.txt and tell the model about missing.txt, which is a partial-success shape the tool has no precedent for. With option 2 the whole call fails, which is consistent with every other tool but loses the load that would have worked. Your repro covers a single bad name, so both pass it.

Tell me which contract to change and I will open the PR against it, including whichever test the change requires — I would just rather have that decided than decided by me.

Thanks for writing this up, and for checking both options against the tests. I went through the code too, and I'd go for a third option.

I'd rule out option 2. Even if Run validated the names, ProcessRequest still does the load, so an artifact deleted in between (or any storage error) would still abort the turn, and we'd pay a second read just to validate.

On option 1, I'd not swallow every failure. AGENTS.md ("Logging and error messages") says library code can't write to a process-wide logger and should return the error to the caller instead, so skipping a storage error would hide it from whoever runs the agent. I think the split should be between mistakes the model can fix and failures it can't.

About the test: all three assertions in TestLoadArtifactsTool_ProcessRequest_LoadError were added together in #1574, whose description is only about the double wrapping. So I read the first one as a snapshot of the current behaviour, not as a decision. It can stay true for storage errors.

On partial success: adk-python and adk-js skip an artifact that isn't found and keep loading the rest, and #866 degrades per artifact too. So loading doc1.txt and telling the model about missing.txt has precedent.

What I'd propose, in processLoadArtifactsFunctionCall:

If Load fails with fs.ErrNotExist or the name is invalid, append a short user content for the model ("X was not found", "X is not a valid artifact name, names are plain filenames") and keep loading the others.
Any other error, including a cancelled context, is returned as today, with the single wrapping from fix(loadartifactstool): stop double-wrapping the artifact load error #1574.
Run stays untouched.

One thing I'd like a maintainer's view on: the invalid-name error from artifact.Service has no sentinel, and the doc of Service.Load doesn't mention fs.ErrNotExist (both built-in services do return it). I'd wrap the file-name validation error with fs.ErrInvalid (no new API) and add one line to the Load doc. The alternative is a local check for / and \ in the tool. Which one do you prefer?

For tests I'd keep LoadError but drive it with a service that fails with a non-NotExist error, so its three assertions stay as they are. The two current cases move to a new test, plus a src/main.go case and a cancelled-context case. The behaviour change goes in the PR description.

#1688 and #866 touch the same function, so I'd keep the change small. Whoever picks it up, I'm happy to discuss the details here.

Opened #1893 against the design you proposed, with your split: fs.ErrNotExist and a path separator are reported to the model and the rest still load; every other error, cancelled context included, is still returned; Run untouched.

On your open question I went with the local check rather than the fs.ErrInvalid sentinel, and I want to flag why since it was the other way from your leaning. Wrapping validateFileName changes err.Error(), and artifact/request_validation_test.go compares that message exactly in five places:

if err != nil && err.Error() != tc.wantErrMsg {

So the sentinel route means five expectation changes plus the Load doc line, which is a bigger change than it looks and not the one this issue is about. I think the sentinel is the better design and I'd send it separately if a maintainer wants it — it just was not worth coupling to this fix.

One assertion in LoadError did not survive as-is, and I would rather tell you than have you find it: it now asserts errors.Is(err, storageErr) for the injected failure instead of fs.ErrNotExist, because the whole point of the change is that fs.ErrNotExist is no longer fatal. The other two — error returned, failure named exactly once — are unchanged, and the double-wrapping concern from #1574 is what they test.

Tests are the three you described, with src/main.go and a cancelled-context case. Reverting the fix fails the new test; restored, the package suite plus ./internal/llminternal and ./tool/... pass.

Thanks for the PR and for spelling out the one assertion that had to change. That matches what I expected. The local check is fine by me, and I agree it keeps this change to one behaviour. For the record, the five message comparisons wouldn't have to change if the sentinel came from a small error type with the same Error() text and an Unwrap to fs.ErrInvalid, but that's a separate cleanup and not needed here.

Navigation Menu

Repository navigation

load_artifacts: a failed artifact load aborts the whole run instead of going back to the model #1873

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

load_artifacts: a failed artifact load aborts the whole run instead of going back to the model #1873

Description

Activity

sushant-me commented on Oct 16, 2026

irenemarbri commented on Oct 17, 2026

sushant-me commented on Oct 17, 2026

irenemarbri commented on Oct 19, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions