fix: prevent panic when artifact service is not configured

[IsAmos01]

Fix: Prevent Panic When Artifact Service Is Not Configured

Fixes #283

🐛 Problem

When using loadartifactstool without properly configuring an ArtifactService in the runner, the application crashes with a nil pointer dereference panic:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x18 pc=0x100a9eba0]

goroutine 1 [running]:
google.golang.org/adk/internal/toolinternal.(*internalArtifacts).List(...)

This provides a poor developer experience as the error is not caught early and the crash message doesn't clearly explain the misconfiguration.

✅ Solution

Added nil checks in the internalArtifacts wrapper methods to return a descriptive error when the artifact service is not configured.

Changes Made

1. Added nil checks in internal/toolinternal/context.go:

   • List() - checks for nil before calling underlying service
   • Load() - checks for nil before calling underlying service
   • LoadVersion() - checks for nil before calling underlying service
   • Save() - checks for nil before calling underlying service

2. Clear error message:

   artifact service is not configured: please configure ArtifactService in the runner
   

3. Comprehensive test coverage:

   • Unit tests in internal/toolinternal/context_test.go verify error handling for all methods
   • Integration test in tool/loadartifactstool/load_artifacts_tool_test.go ensures graceful error handling

🧪 Testing Plan

Unit Tests

• ✅ TestInternalArtifacts_NilArtifactsService - Tests all four methods (List, Load, LoadVersion, Save) return proper errors when service is nil
• ✅ Verifies error messages contain "artifact service is not configured"

Integration Test

• ✅ TestLoadArtifactsTool_ProcessRequest_NilArtifactService - Ensures ProcessRequest returns error instead of panicking when artifact service is not configured

Regression Testing

• ✅ All existing tests continue to pass
• ✅ Normal operation with properly configured artifact service is unaffected

📝 Testing Evidence

# Unit tests pass
$ cd internal/toolinternal && go test -run TestInternalArtifacts_NilArtifactsService
PASS
ok  	google.golang.org/adk/internal/toolinternal	0.691s

# Integration test passes
$ cd tool/loadartifactstool && go test -run TestLoadArtifactsTool_ProcessRequest_NilArtifactService
PASS
ok  	google.golang.org/adk/tool/loadartifactstool	0.735s

🔍 Code Review Notes

• The fix follows the existing error handling pattern used in SearchMemory() (line 104-106 in context.go)
• Error messages are descriptive and actionable
• No breaking changes - only adds safety checks
• Minimal code changes focused on the specific issue

📚 Related

• Issue Panic when Artifact tool is not properly setup #283: Panic when Artifact tool is not properly setup
• Follows Google Go Style Guide error handling best practices

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[gemini-code-assist]

Summary of Changes

Hello @IsAmos01, 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 addresses a critical issue where the application would crash with a nil pointer dereference if the ArtifactService was not properly configured. The changes introduce robust error handling by adding explicit nil checks, ensuring that the system fails gracefully with an informative error message rather than panicking. This significantly improves developer experience by providing clear guidance on misconfiguration.

Highlights

• Nil Pointer Dereference Prevention: Implemented nil checks across all internalArtifacts wrapper methods (List, Load, LoadVersion, Save) to prevent application panics when the ArtifactService is not configured.
• Improved Error Messaging: When the ArtifactService is not configured, a descriptive error message "artifact service is not configured: please configure ArtifactService in the runner" is now returned instead of a panic.
• Comprehensive Test Coverage: Added new unit tests (TestInternalArtifacts_NilArtifactsService) to verify error handling for all affected methods and an integration test (TestLoadArtifactsTool_ProcessRequest_NilArtifactService) to ensure graceful error handling in the LoadArtifactsTool.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog

• internal/toolinternal/context.go
  • Added nil checks to the Save method for ia.Artifacts.
  • Implemented List, Load, and LoadVersion methods, each including a nil check for ia.Artifacts to return a configuration error.
• internal/toolinternal/context_test.go
  • Introduced TestInternalArtifacts_NilArtifactsService to test the List, Load, LoadVersion, and Save methods when the Artifacts service is explicitly set to nil.
  • Verified that all tested methods return the expected "artifact service is not configured" error message.
  • Added helper functions contains and containsAt for robust substring checking in test assertions.
• tool/loadartifactstool/load_artifacts_tool_test.go
  • Added TestLoadArtifactsTool_ProcessRequest_NilArtifactService to confirm that the ProcessRequest method of LoadArtifactsTool returns an error when the artifact service is not configured, preventing a panic.

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 addresses a panic that occurs when the artifact service is not configured by adding nil checks. The changes are well-tested with both unit and integration tests. I have a couple of suggestions to improve code maintainability and consistency by reducing code duplication.

[gemini-code-assist]

The error message string "artifact service is not configured: please configure ArtifactService in the runner" is duplicated in this function and also in List, Load, and LoadVersion. To improve maintainability and avoid potential inconsistencies, please extract it into a package-level constant.

For example:

const artifactServiceNotConfiguredMsg = "artifact service is not configured: please configure ArtifactService in the runner"

Then you can use fmt.Errorf(artifactServiceNotConfiguredMsg) here and in the other methods.

[IsAmos01]

@google-cla-bot check

[IsAmos01]

@google-cla-bot check

[IsAmos01]

@google-cla-bot check

[IsAmos01]

@google-cla-bot check

[IsAmos01]

@google-cla-bot check

[IsAmos01]

@google-cla-bot check