refactor(tool): unify integrations on gollem-dev/tools, add Jira and Falcon#225
refactor(tool): unify integrations on gollem-dev/tools, add Jira and Falcon#225m-mizutani wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors Warren (v0.18.0) by retiring the legacy pkg/agents sub-agent framework and unifying all external integrations as standard tools under pkg/tool/*. Specifically, CrowdStrike Falcon and Slack search are migrated to standard tools, a new read-only Jira tool is introduced, and the budget tracking logic is simplified. The code review feedback focuses on cleaning up the configuration logic for the new Falcon and Jira tools, recommending the removal of unused fields, the dynamic construction of configuration options inside the Configure methods rather than during flag parsing, and the simplification of associated test helpers.
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.
| type Action struct { | ||
| clientID string | ||
| clientSecret string | ||
| baseURL string | ||
|
|
||
| opts []extfalcon.Option | ||
| inner gollem.ToolSet | ||
| } |
There was a problem hiding this comment.
The opts field in the Action struct is used to accumulate configuration options during flag parsing. However, this is fragile and can lead to duplicate options if flags are parsed multiple times. It is cleaner to construct the options slice dynamically inside the Configure method.
type Action struct {
clientID string
clientSecret string
baseURL string
inner gollem.ToolSet
}| &cli.StringFlag{ | ||
| Name: "falcon-base-url", | ||
| Usage: "CrowdStrike Falcon API base URL", | ||
| Destination: &x.baseURL, | ||
| Category: "Tool", | ||
| Value: defaultBaseURL, | ||
| Sources: cli.EnvVars("WARREN_FALCON_BASE_URL"), | ||
| Action: func(_ context.Context, _ *cli.Command, v string) error { | ||
| x.opts = append(x.opts, extfalcon.WithBaseURL(v)) | ||
| return nil | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Remove the Action callback from the falcon-base-url flag. Appending to a slice in a flag callback is fragile. Instead, we can read the parsed x.baseURL directly in the Configure method.
&cli.StringFlag{
Name: "falcon-base-url",
Usage: "CrowdStrike Falcon API base URL",
Destination: &x.baseURL,
Category: "Tool",
Value: defaultBaseURL,
Sources: cli.EnvVars("WARREN_FALCON_BASE_URL"),
},| func (x *Action) Configure(_ context.Context) error { | ||
| if x.clientID == "" || x.clientSecret == "" { | ||
| return errutil.ErrActionUnavailable | ||
| } | ||
|
|
||
| ts, err := extfalcon.New(x.clientID, x.clientSecret, x.opts...) | ||
| if err != nil { | ||
| return goerr.Wrap(err, "failed to configure Falcon tool") | ||
| } | ||
| x.inner = ts | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Construct the opts slice dynamically inside Configure using x.baseURL instead of relying on the flag callback.
func (x *Action) Configure(_ context.Context) error {
if x.clientID == "" || x.clientSecret == "" {
return errutil.ErrActionUnavailable
}
var opts []extfalcon.Option
if x.baseURL != "" {
opts = append(opts, extfalcon.WithBaseURL(x.baseURL))
}
ts, err := extfalcon.New(x.clientID, x.clientSecret, opts...)
if err != nil {
return goerr.Wrap(err, "failed to configure Falcon tool")
}
x.inner = ts
return nil
}| // SetTestURL points the underlying toolset at a stub server (test-only). | ||
| func (x *Action) SetTestURL(url string) { | ||
| x.opts = append(x.opts, extfalcon.WithBaseURL(url)) | ||
| } |
There was a problem hiding this comment.
Since we removed the opts field from the Action struct, simplify SetTestURL to set x.baseURL directly.
| // SetTestURL points the underlying toolset at a stub server (test-only). | |
| func (x *Action) SetTestURL(url string) { | |
| x.opts = append(x.opts, extfalcon.WithBaseURL(url)) | |
| } | |
| // SetTestURL points the underlying toolset at a stub server (test-only). | |
| func (x *Action) SetTestURL(url string) { | |
| x.baseURL = url | |
| } |
| type Action struct { | ||
| baseURL string | ||
| email string | ||
| apiToken string | ||
|
|
||
| opts []extjira.Option | ||
| inner gollem.ToolSet | ||
| } |
There was a problem hiding this comment.
The opts field in the Action struct is completely unused. We can remove it to simplify the struct.
type Action struct {
baseURL string
email string
apiToken string
inner gollem.ToolSet
}| func (x *Action) Configure(_ context.Context) error { | ||
| if x.baseURL == "" || x.email == "" || x.apiToken == "" { | ||
| return errutil.ErrActionUnavailable | ||
| } | ||
|
|
||
| ts, err := extjira.New(x.baseURL, x.email, x.apiToken, x.opts...) | ||
| if err != nil { | ||
| return goerr.Wrap(err, "failed to configure Jira tool") | ||
| } | ||
| x.inner = ts | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Remove the unused x.opts... argument from the extjira.New call.
| func (x *Action) Configure(_ context.Context) error { | |
| if x.baseURL == "" || x.email == "" || x.apiToken == "" { | |
| return errutil.ErrActionUnavailable | |
| } | |
| ts, err := extjira.New(x.baseURL, x.email, x.apiToken, x.opts...) | |
| if err != nil { | |
| return goerr.Wrap(err, "failed to configure Jira tool") | |
| } | |
| x.inner = ts | |
| return nil | |
| } | |
| func (x *Action) Configure(_ context.Context) error { | |
| if x.baseURL == "" || x.email == "" || x.apiToken == "" { | |
| return errutil.ErrActionUnavailable | |
| } | |
| ts, err := extjira.New(x.baseURL, x.email, x.apiToken) | |
| if err != nil { | |
| return goerr.Wrap(err, "failed to configure Jira tool") | |
| } | |
| x.inner = ts | |
| return nil | |
| } |
1 participant
Summary
Unifies all external integrations under
pkg/tool/*(backed bygithub.com/gollem-dev/tools) and retires the legacypkg/agents"sub-agent" framework.gollem-dev/toolsslack/github/webfetchto v0.2.0; addfalcon/jira.pkg/tool/falcon(CrowdStrike Falcon,WARREN_FALCON_*) andpkg/tool/jira(Jira Cloud,WARREN_JIRA_*), both read-only wrappers overgollem-dev/tools, registered inpkg/cli/utils.go.pkg/agentsentirely: the Falcon, Slack, and BigQuery sub-agents plus the framework (agents.go/factory.go) and theirserve/chatwiring. Falcon →pkg/tool/falcon; Slack → existingpkg/tool/slack; BigQuery → existingpkg/tool/bigquery(a functional superset of the old agent).subAgentToolNamesand the unused context-tracker sharing machinery (withBudgetTracker/budgetTrackerFrom/newContextAwareBudgetMiddleware) in bothasterandbluebell. The per-task fixed-tracker path is unchanged.README.md/alert-investigation.md/configuration.md(including the previously-undocumented BigQuery tool settings), removed the stale "Agent Memory" section, and addeddoc/migration/v0.18.0.md.Breaking changes (configuration only — no data migration)
Operators must rename environment variables:
WARREN_AGENT_FALCON_*WARREN_FALCON_*WARREN_AGENT_SLACK_USER_TOKENWARREN_SLACK_TOOL_USER_TOKENWARREN_AGENT_BIGQUERY_*WARREN_BIGQUERY_*See
doc/migration/v0.18.0.mdfor full details.Verification
go vet ./...— cleango test ./...— all passgolangci-lint run ./...— 0 issuesgosec— no findingsKnown follow-up
.claude/rules/subagent-development.mdwas removed, but.claude/rules/documentation-updates.mdstill references the removed sub-agent flow — that edit was blocked by the self-modification guard and needs to be applied separately.🤖 Generated with Claude Code