Skip to content

refactor(tool): adopt gollem type-safe NewTool API and bump gollem/tools deps#226

Merged
m-mizutani merged 3 commits into
mainfrom
refactor/gollem-type-safe-adoption
Jul 1, 2026
Merged

refactor(tool): adopt gollem type-safe NewTool API and bump gollem/tools deps#226
m-mizutani merged 3 commits into
mainfrom
refactor/gollem-type-safe-adoption

Conversation

@m-mizutani

Copy link
Copy Markdown
Contributor

Summary

Adopts the type-safe tool-construction API that github.com/gollem-dev/gollem (main) and github.com/gollem-dev/tools/* (v0.2.0 / v0.3.0) now provide, and applies it across warren's own hand-written tools.

Dependency bump

  • gollem → main HEAD (purely additive: typed_tool.go + ErrInvalidToolType, no breaking changes)
  • tools 12 modules → v0.3.0 for github / slack / webfetch, v0.2.0 for the rest
  • External wrappers in pkg/tool/<name>/action.go need no code change — every constructor / Option / With* signature is unchanged; the bumps are internal-only.

Type-safe migration of warren's own tools (7 files / ~23 tools)

  • New shared adapter pkg/utils/toolset converts []gollem.Tool (built via gollem.NewTool[In,Out]) into the gollem.ToolSet (Specs/Run) contract that interfaces.ToolSet requires.
  • Migrated: refine (1), tool/base (6), agents/falcon (10), agents/slack (3), agents/bigquery (2–3, runbook conditional), tool/bigquery config generator (2), tool/knowledge (up to 11, mode-dependent).
  • Removes the getArg / args[...].(T) / buildQueryParams boilerplate and manual float64int64 special-casing; schemas are now inferred from typed input structs.

webfetch SSRF

  • webfetch v0.2.0+ blocks private/internal IPs by default. Per design decision, the new default is adopted (no opt-out wired). pkg/tool/webfetch/README.md updated to reflect this.

Design notes

  • interfaces.ToolSet (Specs/Run + ID/Description/Prompt) shape is preserved because the planner groups tools by ToolSet; only each set's internals are made type-safe.
  • falcon/slack pagination fields stay float64 so the inferred JSON schema remains number, matching the prior wire-level type.
  • gollem discards a tool's result map whenever the tool returns an error, so migrating generate_config's (map, ErrExitConversation) path to NewTool does not change observable behavior.

Test

  • go test ./... — all packages pass (github/slack wrapper spec-count assertions updated for the new tools github_get_issue / github_get_pull_request / slack_get_messages).
  • go vet ./..., gofmt -l, golangci-lint run (0 issues), gosec — all clean.
  • New unit tests for pkg/utils/toolset (ordering, dispatch, unknown-tool error, duplicate-name panic, int64 decode).

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the tool and agent implementations across BigQuery, Falcon, Slack, Warren, and Knowledge packages to utilize a new type-safe toolset utility (pkg/utils/toolset). This change replaces manual map parsing and hand-written tool specifications with structured input types parsed by gollem. The review feedback highlights several opportunities for defensive programming, recommending nil checks on configurations, captured context variables, and toolset references to prevent potential runtime panics.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +61 to +66
func newInternalTool(config *Config, projectID, impersonateServiceAccount string) *internalTool {
t := &internalTool{
config: config,
projectID: projectID,
impersonateServiceAccount: impersonateServiceAccount,
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Defensive programming: config is a pointer and is dereferenced later in the function (e.g., config.Tables and config.Runbooks). If config is nil, this will cause a runtime panic. Consider adding a nil check at the beginning of newInternalTool to prevent this.

func newInternalTool(config *Config, projectID, impersonateServiceAccount string) *internalTool {
	if config == nil {
		panic("config is required")
	}
	t := &internalTool{
		config:                    config,
		projectID:                 projectID,
		impersonateServiceAccount: impersonateServiceAccount,
	}

Comment thread pkg/usecase/refine.go
Comment on lines +243 to +246
func(ctx context.Context, in slackPostMessageInput) (map[string]any, error) {
if in.Message == "" {
return map[string]any{"error": "message parameter is required and must be a non-empty string"}, nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Defensive programming: The tool handler closure captures uc and t from the outer scope. If either uc or t is nil, dereferencing them (e.g., uc.slackService or t.SlackThread) will cause a runtime panic. Consider adding a nil check at the beginning of the handler function.

Suggested change
func(ctx context.Context, in slackPostMessageInput) (map[string]any, error) {
if in.Message == "" {
return map[string]any{"error": "message parameter is required and must be a non-empty string"}, nil
}
func(ctx context.Context, in slackPostMessageInput) (map[string]any, error) {
if uc == nil || t == nil {
return nil, goerr.New("invalid usecase or ticket reference")
}
if in.Message == "" {
return map[string]any{"error": "message parameter is required and must be a non-empty string"}, nil
}

Comment on lines +40 to +41
for _, t := range tools {
name := t.Spec().Name

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Defensive programming: If any of the tools passed to New is nil, calling t.Spec() will cause a nil pointer dereference panic. Consider adding a nil check to handle this programming error gracefully.

Suggested change
for _, t := range tools {
name := t.Spec().Name
for _, t := range tools {
if t == nil {
panic(goerr.New("nil tool provided to toolset", goerr.T(errutil.TagInvalidState)))
}
name := t.Spec().Name

Comment on lines +52 to +53
func (s *Set) Specs(_ context.Context) ([]gollem.ToolSpec, error) {
specs := make([]gollem.ToolSpec, 0, len(s.order))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Defensive programming: If the Set pointer s or its underlying tools map is nil, calling Specs will cause a runtime panic. Consider adding a nil check at the beginning of Specs.

Suggested change
func (s *Set) Specs(_ context.Context) ([]gollem.ToolSpec, error) {
specs := make([]gollem.ToolSpec, 0, len(s.order))
func (s *Set) Specs(_ context.Context) ([]gollem.ToolSpec, error) {
if s == nil || s.tools == nil {
return nil, nil
}
specs := make([]gollem.ToolSpec, 0, len(s.order))

Comment on lines +61 to +62
func (s *Set) Run(ctx context.Context, name string, args map[string]any) (map[string]any, error) {
t, ok := s.tools[name]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Defensive programming: If the Set pointer s or its underlying tools map is nil, calling Run will cause a runtime panic. Consider adding a nil check at the beginning of Run.

Suggested change
func (s *Set) Run(ctx context.Context, name string, args map[string]any) (map[string]any, error) {
t, ok := s.tools[name]
func (s *Set) Run(ctx context.Context, name string, args map[string]any) (map[string]any, error) {
if s == nil || s.tools == nil {
return nil, goerr.New("toolset is not initialized", goerr.T(errutil.TagInvalidState))
}
t, ok := s.tools[name]

@m-mizutani
m-mizutani merged commit d213e76 into main Jul 1, 2026
4 checks passed
@m-mizutani
m-mizutani deleted the refactor/gollem-type-safe-adoption branch July 1, 2026 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant