fix: prevent panic when loadartifactstool used without artifact service

[ShammiAnand]

Fixes #283

Root Cause

internalArtifacts wrapper created with nil embedded Artifacts field. Calling List(), Load(), or Save() on nil receiver caused segfault.

Changes

• internal/toolinternal/context.go: Added nil checks to List/Load/Save methods, only create wrapper if service configured
• tool/loadartifactstool/load_artifacts_tool_test.go: Test for error when service missing
• internal/toolinternal/context_test.go: Tests for nil-safe methods

Result

Before: panic: runtime error: invalid memory address or nil pointer dereference
After: ERROR: failed to list artifacts: artifact service not configured

[gemini-code-assist]

Summary of Changes

Hello @ShammiAnand, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a critical issue where the loadartifactstool could lead to a runtime panic if it attempted to operate without a configured artifact service. The changes introduce robust checks and conditional logic to ensure that any operations on a missing service now result in a clear error message, significantly improving the stability and predictability of the tool's behavior.

Highlights

• Panic Prevention: Implemented nil checks within the internalArtifacts wrapper's List, Load, and Save methods to prevent runtime panics when the underlying artifact service is not configured.
• Conditional Initialization: Modified the NewToolContext function to conditionally initialize the internalArtifacts wrapper only if an artifact service is present, avoiding the creation of a wrapper with a nil embedded Artifacts field.
• Enhanced Test Coverage: Added new unit tests (TestInternalArtifacts_NilSafe and TestLoadArtifactsTool_ProcessRequest_NoArtifactService) to specifically verify the graceful error handling and nil-safe behavior when the artifact service is unavailable.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request effectively resolves a panic that occurred when the artifact service was not configured by introducing nil checks and conditionally creating the artifact wrapper. The inclusion of tests to cover this fix is a great addition. I've provided a couple of suggestions to further enhance the code by improving maintainability, reducing duplication, and adhering to Go's error handling best practices.

[ShammiAnand]

@baptmont can you please review this?

[verdverm]

I would think it should be possible to catch this sooner, like when the agent is configured or starts up... run a few sanity checks

Right now this feels more like a bandaid, catching at the point of failure artifact.Load/Save/List. Better than it is today, but not really solving the issue of misconfiguration until I actually try to use the artifact service

[ShammiAnand]

@verdverm

Added runner-level validation in runner.New() that:

• Walks entire agent tree checking for load_artifacts tool
• Returns error immediately if tool present but ArtifactService not configured
• Validates during runner creation (proactive) vs at tool invocation (reactive)

Before: Error at runtime when tool called
panic: runtime error: invalid memory address or nil pointer dereference

After: Error at runner creation
Failed to create runner: agent "artifact_tester" uses load_artifacts tool but ArtifactService not configured in runner

[ShammiAnand]

@verdverm can we merge it?

[verdverm]

@ShammiAnand I have no control over that, just an ADK user

[verdverm]

@ShammiAnand well, this is less likely to be merged with ~1.5M LOC being contributed

what is going on with dd35910

[kdroste-google]

@ShammiAnand, why do you need vendor/... directory? It adds huge amount of code, and I think your intention was just to modify 5 files...

[mazas-google]

+1, please drop vendor directory.

[ShammiAnand]

@kdroste-google @mazas-google done!

[ShammiAnand]

I defined the Validator interface and ValidationConfig struct in the agent package, which serves as the common definition:

// agent/agent.go
type ValidationConfig struct {
	HasArtifactService bool
	HasMemoryService   bool
}
type Validator interface {
	Validate(ValidationConfig) error
}

• Agents: The agent struct implements this interface by delegating to the Validate function provided in its Config.
• Tools: Tools (like load_artifacts_tool) import the agent package and implement the Validate(agent.ValidationConfig) error method directly.

The runner then checks for this single interface on both entities during initialization.

@kdroste-google

[dpasiukevich]

This is a good change, thanks!

Given that the PR introduces a general solution, WDYT if we follow in this pattern:

1. packages agent.Config and llmagent.Config would instead expose ValidateFunc func() error which agent.New and llmagent.New will run. This will allow agent creators to define any requirements and run validation they need for their agents.

Because ValidationConfig will be a weak spot right now. It will grow to a huge struct as other people would ask in future to add more fields to run validation. Better to give the flexibility from the start.

2. As a helper to run environment runtime validation, the runner package will only expose

package runner

type Validator interface {
	Validate(Config) error
}

And validateConfiguration is kept as it's in this PR -- it will verify if the agent or a tool implement runner.Validator interface, then it runs it.

e.g. right now loadartifacttool can implement the runner.Validator interface.

[ShammiAnand]

This is a good change, thanks!

Given that the PR introduces a general solution, WDYT if we follow in this pattern:

1. packages `agent.Config` and `llmagent.Config` would instead expose `ValidateFunc func() error` which `agent.New` and `llmagent.New` will run. This will allow agent creators to define any requirements and run validation they need for their agents.

Because ValidationConfig will be a weak spot right now. It will grow to a huge struct as other people would ask in future to add more fields to run validation. Better to give the flexibility from the start.

2. As a helper to run environment runtime validation, the `runner` package will only expose

package runner

type Validator interface {
	Validate(Config) error
}

And validateConfiguration is kept as it's in this PR -- it will verify if the agent or a tool implement runner.Validator interface, then it runs it.

e.g. right now loadartifacttool can implement the runner.Validator interface.

I have refactored the validation logic as suggested.

• Moved ValidateFunc to agent.Config and llmagent.Config for creation-time validation.
• Introduced runner.Validator interface for runtime validation of runner configuration by agents/tools.
• Updated loadartifactstool to implement runner.Validator.
• All tests passed.

@dpasiukevich @kdroste-google

[verdverm]

This Validation extension to the interfaces seems useful to have across all tools, as they may rely on some configuration and services on a case-by-case basis. How much effort would it be to expand this beyond the Agent tool and specificity?

[wolo-lab]

Thank you for working on it. Closing as duplicate. The PR #1390 is going to solve #283.
We might get back to the idea of having more general validator as a follow-up.