adk-go icon indicating copy to clipboard operation
adk-go copied to clipboard

fix: prevent panic when loadartifactstool used without artifact service

Open ShammiAnand opened this issue 2 weeks ago • 11 comments

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

ShammiAnand avatar Nov 14 '25 09:11 ShammiAnand

Summary of Changes

Hello @ShammiAnand, I'm Gemini Code Assist[^1]! 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 :thumbsup: and :thumbsdown: 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.

[^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[bot] avatar Nov 14 '25 09:11 gemini-code-assist[bot]

@baptmont can you please review this?

ShammiAnand avatar Nov 14 '25 16:11 ShammiAnand

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

verdverm avatar Nov 16 '25 18:11 verdverm

@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 avatar Nov 17 '25 17:11 ShammiAnand

@verdverm can we merge it?

ShammiAnand avatar Nov 19 '25 06:11 ShammiAnand

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

verdverm avatar Nov 20 '25 23:11 verdverm

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

what is going on with dd35910

verdverm avatar Nov 21 '25 20:11 verdverm

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

kdroste-google avatar Nov 24 '25 11:11 kdroste-google

+1, please drop vendor directory.

mazas-google avatar Nov 24 '25 21:11 mazas-google

@kdroste-google @mazas-google done!

ShammiAnand avatar Nov 25 '25 13:11 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

ShammiAnand avatar Nov 26 '25 07:11 ShammiAnand