fix(client): normalize toolFilter allow-list entries - #68
Conversation
Parse comma-separated toolFilter.list values and trim whitespace so env-expanded lists like ENABLED_TOOLS=a,b,c match individual tool names. Fixes tbxark#53
![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 the tool filtering logic by extracting it into a dedicated helper function buildToolFilterFunc and adds support for parsing toolFilter.list as either a JSON array or a comma-separated string to accommodate environment-variable expansion. It also introduces comprehensive unit tests and updates the configuration documentation. The review feedback suggests improving efficiency and robustness by validating the filter mode early in buildToolFilterFunc to avoid unnecessary processing, and adding validation during JSON unmarshaling to prevent invalid modes from silently disabling the filter.
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.
| filterSet := make(map[string]struct{}, len(filterList)) | ||
| mode := ToolFilterMode(strings.ToLower(string(options.ToolFilter.Mode))) | ||
| for _, toolName := range filterList { | ||
| filterSet[toolName] = struct{}{} | ||
| } | ||
| log.Printf("<%s> Applying toolFilter mode=%s for tools: %s", clientName, mode, strings.Join(filterList, ", ")) | ||
|
|
||
| switch mode { | ||
| case ToolFilterModeAllow: | ||
| return func(toolName string) bool { | ||
| _, inList := filterSet[toolName] | ||
| if !inList { | ||
| log.Printf("<%s> Ignoring tool %s as it is not in allow list", clientName, toolName) | ||
| } | ||
| case ToolFilterModeBlock: | ||
| filterFunc = func(toolName string) bool { | ||
| _, inList := filterSet[toolName] | ||
| if inList { | ||
| log.Printf("<%s> Ignoring tool %s as it is in block list", c.name, toolName) | ||
| } | ||
| return !inList | ||
| return inList | ||
| } | ||
| case ToolFilterModeBlock: | ||
| return func(toolName string) bool { | ||
| _, inList := filterSet[toolName] | ||
| if inList { | ||
| log.Printf("<%s> Ignoring tool %s as it is in block list", clientName, toolName) | ||
| } | ||
| default: | ||
| log.Printf("<%s> Unknown tool filter mode: %s, skipping tool filter", c.name, mode) | ||
| return !inList | ||
| } | ||
| default: | ||
| log.Printf("<%s> Unknown tool filter mode: %s, skipping tool filter", clientName, mode) | ||
| return filterFunc | ||
| } |
There was a problem hiding this comment.
If the filter mode is invalid or unknown, the code currently builds the filterSet and logs a message stating that it is applying the filter before finally logging that the mode is unknown and skipping it. It is cleaner and more efficient to validate the mode first, logging the error and returning early if it is invalid.
mode := ToolFilterMode(strings.ToLower(string(options.ToolFilter.Mode)))
if mode != ToolFilterModeAllow && mode != ToolFilterModeBlock {
log.Printf("<%s> Unknown tool filter mode: %s, skipping tool filter", clientName, mode)
return filterFunc
}
filterSet := make(map[string]struct{}, len(filterList))
for _, toolName := range filterList {
filterSet[toolName] = struct{}{}
}
log.Printf("<%s> Applying toolFilter mode=%s for tools: %s", clientName, mode, strings.Join(filterList, ", "))
switch mode {
case ToolFilterModeAllow:
return func(toolName string) bool {
_, inList := filterSet[toolName]
if !inList {
log.Printf("<%s> Ignoring tool %s as it is not in allow list", clientName, toolName)
}
return inList
}
case ToolFilterModeBlock:
return func(toolName string) bool {
_, inList := filterSet[toolName]
if inList {
log.Printf("<%s> Ignoring tool %s as it is in block list", clientName, toolName)
}
return !inList
}
default:
return filterFunc
}| if err := json.Unmarshal(data, &aux); err != nil { | ||
| return err | ||
| } | ||
| c.Mode = ToolFilterMode(strings.TrimSpace(string(aux.Mode))) |
There was a problem hiding this comment.
The conversion string(aux.Mode) is redundant since aux.Mode is already a string. Additionally, we should validate that the unmarshaled mode is either allow, block, or empty to prevent typos or invalid configurations from silently disabling the filter and exposing all tools (fail-open behavior).
| c.Mode = ToolFilterMode(strings.TrimSpace(string(aux.Mode))) | |
| mode := strings.ToLower(strings.TrimSpace(aux.Mode)) | |
| if mode != "" && mode != string(ToolFilterModeAllow) && mode != string(ToolFilterModeBlock) { | |
| return fmt.Errorf("invalid toolFilter.mode: %q (must be 'allow' or 'block')", aux.Mode) | |
| } | |
| c.Mode = ToolFilterMode(mode) |
1 participant
Summary
Normalize
toolFilter.listentries before allow/block matching so comma-separated values and surrounding whitespace do not cause valid tools to be filtered out.Fixes #53
Motivation
Issue #53 reports that tools explicitly listed in an allow-list (e.g.
jira_search) are still rejected. A common cause is env-expanded config where the list collapses to a single comma-separated string (e.g.["confluence_search,jira_get_issue,jira_search"]), which previously matched only the full string and not individual tool names. Trailing/leading whitespace on entries can cause the same symptom.Changes
normalizeToolFilterListto trim, split comma-separated values, and dedupe entriestoolFilter.listas a JSON array or a single comma-separated string via customUnmarshalJSONbuildToolFilterFuncfor testability and log the effective filter set at startuplistsupport indocs/CONFIGURATION.mdTests
go test ./... -count=1— passgo vet ./...— passNotes