feat(tools/shell): add runtime-reloadable global shell deny-groups via config.tools

Adds ToolsConfig.ShellDenyGroups (map[string]bool) so operators can toggle
shell deny groups (package_install, env_dump, etc.) from the /config Web UI
without a process restart.

Wiring:
- ExecTool gains globalDenyGroups + SetGlobalShellDenyGroups (defensive copy).
- effectiveDenyGroups merges per-agent context overrides with global, with
  per-key agent precedence — multi-tenant invariant preserved.
- setupToolRegistry applies the initial config before any request can arrive.
- subscribeShellDenyGroupsReload (extracted helper for testability) reacts to
  bus.TopicConfigChanged and re-applies the new toggles.

Regression coverage the original PR #1005 attempt missed:
- 6 unit tests for the merge / defensive-copy semantics in internal/tools.
- 3 pub/sub dispatch tests in cmd: happy path, wrong event name, wrong
  payload type.
This commit is contained in:
viettranx committed 2026-04-24 20:59:08 +07:00
1 parent 7ffcdf16d1
commit df5d48c78d
7 files changed
+285 -4

No files matched your search

+4
View File
@@ -84,6 +84,10 @@ func (d *gatewayDeps) runLifecycle(
deps.webFetchTool.UpdatePolicy(updatedCfg.Tools.WebFetch.Policy, updatedCfg.Tools.WebFetch.AllowedDomains, updatedCfg.Tools.WebFetch.BlockedDomains)
})
// Reload global shell deny-group toggles on config changes via pub/sub
// so /config edits apply without a process restart.
subscribeShellDenyGroupsReload(d.msgBus, d.toolsReg)
// Reload TTS providers on config changes via pub/sub.
d.msgBus.Subscribe("tts-config-reload", func(evt bus.Event) {
if evt.Name != bus.TopicConfigChanged {
@@ -0,0 +1,35 @@
package cmd
import (
"log/slog"
"github.com/nextlevelbuilder/goclaw/internal/bus"
"github.com/nextlevelbuilder/goclaw/internal/config"
"github.com/nextlevelbuilder/goclaw/internal/tools"
)
// subscribeShellDenyGroupsReload wires pub/sub so global shell deny-group
// toggles applied via the /config page take effect without a process restart.
// Extracted from runLifecycle to make the dispatch path unit-testable
// (the regression coverage missing from the original PR #1005 attempt).
func subscribeShellDenyGroupsReload(msgBus *bus.MessageBus, toolsReg *tools.Registry) {
msgBus.Subscribe("shell-deny-groups-config-reload", func(evt bus.Event) {
if evt.Name != bus.TopicConfigChanged {
return
}
updatedCfg, ok := evt.Payload.(*config.Config)
if !ok {
return
}
execTool, ok := toolsReg.Get("exec")
if !ok {
return
}
et, ok := execTool.(*tools.ExecTool)
if !ok {
return
}
et.SetGlobalShellDenyGroups(updatedCfg.Tools.ShellDenyGroups)
slog.Info("shell deny groups reloaded via pub/sub", "groups", len(updatedCfg.Tools.ShellDenyGroups))
})
}
@@ -0,0 +1,90 @@
package cmd
import (
"context"
"testing"
"github.com/nextlevelbuilder/goclaw/internal/bus"
"github.com/nextlevelbuilder/goclaw/internal/config"
"github.com/nextlevelbuilder/goclaw/internal/tools"
)
// TestShellDenyGroupsConfigReload_UpdatesGlobal asserts the pub/sub subscriber
// dispatches a TopicConfigChanged event into ExecTool.SetGlobalShellDenyGroups —
// the regression coverage that the original PR #1005 was missing.
func TestShellDenyGroupsConfigReload_UpdatesGlobal(t *testing.T) {
msgBus := bus.New()
defer msgBus.Unsubscribe("shell-deny-groups-config-reload")
toolsReg := tools.NewRegistry()
execTool := tools.NewExecTool("/tmp", false)
toolsReg.Register(execTool)
subscribeShellDenyGroupsReload(msgBus, toolsReg)
msgBus.Broadcast(bus.Event{
Name: bus.TopicConfigChanged,
Payload: &config.Config{
Tools: config.ToolsConfig{
ShellDenyGroups: map[string]bool{"package_install": true},
},
},
})
got := execTool.EffectiveDenyGroupsForTest(context.Background())
if v, ok := got["package_install"]; !ok || v != true {
t.Fatalf("expected pub/sub to set global package_install=true, got %v", got)
}
}
// TestShellDenyGroupsConfigReload_IgnoresOtherEvents: subscriber must guard
// on event.Name and ignore non-TopicConfigChanged broadcasts.
func TestShellDenyGroupsConfigReload_IgnoresOtherEvents(t *testing.T) {
msgBus := bus.New()
defer msgBus.Unsubscribe("shell-deny-groups-config-reload")
toolsReg := tools.NewRegistry()
execTool := tools.NewExecTool("/tmp", false)
execTool.SetGlobalShellDenyGroups(map[string]bool{"package_install": false}) // baseline
toolsReg.Register(execTool)
subscribeShellDenyGroupsReload(msgBus, toolsReg)
msgBus.Broadcast(bus.Event{
Name: bus.TopicAgentDeleted,
Payload: &config.Config{
Tools: config.ToolsConfig{
ShellDenyGroups: map[string]bool{"package_install": true},
},
},
})
got := execTool.EffectiveDenyGroupsForTest(context.Background())
if v := got["package_install"]; v != false {
t.Fatalf("expected non-config event to be ignored; package_install changed to %v", v)
}
}
// TestShellDenyGroupsConfigReload_IgnoresWrongPayload: subscriber must
// type-assert payload to *config.Config and skip mismatched payloads.
func TestShellDenyGroupsConfigReload_IgnoresWrongPayload(t *testing.T) {
msgBus := bus.New()
defer msgBus.Unsubscribe("shell-deny-groups-config-reload")
toolsReg := tools.NewRegistry()
execTool := tools.NewExecTool("/tmp", false)
execTool.SetGlobalShellDenyGroups(map[string]bool{"package_install": false})
toolsReg.Register(execTool)
subscribeShellDenyGroupsReload(msgBus, toolsReg)
msgBus.Broadcast(bus.Event{
Name: bus.TopicConfigChanged,
Payload: "not-a-config-pointer",
})
got := execTool.EffectiveDenyGroupsForTest(context.Background())
if v := got["package_install"]; v != false {
t.Fatalf("expected wrong-payload event to be ignored; package_install changed to %v", v)
}
}
+3
View File
@@ -211,6 +211,9 @@ func setupToolRegistry(
// Exception: .goclaw/skills-store/ is allowed (skills may contain executable scripts).
if execTool, ok := toolsReg.Get("exec"); ok {
if et, ok := execTool.(*tools.ExecTool); ok {
// Apply global shell deny-group toggles before any request can arrive.
// Per-agent overrides via store.WithShellDenyGroups still win per-key.
et.SetGlobalShellDenyGroups(cfg.Tools.ShellDenyGroups)
et.DenyPaths(dataDir, ".goclaw/")
// Allow skills execution: master-tenant skills-store + all tenant-scoped skills-store dirs.
et.AllowPathExemptions(
+3 -2
View File
@@ -369,8 +369,9 @@ type ToolsConfig struct {
Allow []string `json:"allow,omitempty"` // global allow list (tool names or "group:xxx")
Deny []string `json:"deny,omitempty"` // global deny list
AlsoAllow []string `json:"alsoAllow,omitempty"` // additive: adds without removing existing
ByProvider map[string]*ToolPolicySpec `json:"byProvider,omitempty"` // per-provider overrides
ExecApproval ExecApprovalCfg `json:"execApproval"` // exec command approval settings
ByProvider map[string]*ToolPolicySpec `json:"byProvider,omitempty"` // per-provider overrides
ShellDenyGroups map[string]bool `json:"shellDenyGroups,omitempty"` // global shell deny-group toggles (group name -> denied); per-agent overrides win per-key
ExecApproval ExecApprovalCfg `json:"execApproval"` // exec command approval settings
WebFetch WebFetchPolicyConfig `json:"web_fetch"` // domain policy for URL fetching
Browser BrowserToolConfig `json:"browser"`
RateLimitPerHour int `json:"rate_limit_per_hour,omitempty"` // max tool executions per hour per session (0 = disabled)
+50 -2
View File
@@ -43,6 +43,53 @@ type ExecTool struct {
approvalMgr *ExecApprovalManager // nil = no approval needed
agentID string // for approval request context
secureCLIStore store.SecureCLIStore // nil = no credentialed exec
// globalDenyGroups holds global shell deny-group toggles from config.tools.
// Per-agent overrides from context (store.WithShellDenyGroups) win per-key.
// Updated at startup and via TopicConfigChanged pub/sub for runtime reload.
globalDenyGroups map[string]bool
}
// SetGlobalShellDenyGroups replaces the global shell deny-group toggles. The
// caller's map is defensively copied so later mutations cannot leak into the
// tool's internal state. Passing nil or an empty map clears the global config
// (per-agent context overrides, if any, still apply on their own).
func (t *ExecTool) SetGlobalShellDenyGroups(groups map[string]bool) {
if len(groups) == 0 {
t.globalDenyGroups = nil
return
}
cp := make(map[string]bool, len(groups))
for k, v := range groups {
cp[k] = v
}
t.globalDenyGroups = cp
}
// effectiveDenyGroups merges the per-agent context override with the global
// config. Precedence: per-agent context (per-key) > global. When one side is
// empty, the other is returned directly (no allocation).
func (t *ExecTool) effectiveDenyGroups(ctx context.Context) map[string]bool {
agent := store.ShellDenyGroupsFromContext(ctx)
if len(t.globalDenyGroups) == 0 {
return agent
}
if len(agent) == 0 {
return t.globalDenyGroups
}
merged := make(map[string]bool, len(t.globalDenyGroups)+len(agent))
for k, v := range t.globalDenyGroups {
merged[k] = v
}
for k, v := range agent {
merged[k] = v // agent wins per-key
}
return merged
}
// EffectiveDenyGroupsForTest exposes effectiveDenyGroups for cross-package tests
// (e.g. cmd pub/sub regression). Not for production callers.
func (t *ExecTool) EffectiveDenyGroupsForTest(ctx context.Context) map[string]bool {
return t.effectiveDenyGroups(ctx)
}
// NewExecTool creates an exec tool that runs commands directly on the host.
@@ -152,8 +199,9 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *Result {
// Unicode-based pattern bypass while preserving functional command content.
normalizedCommand := normalizeCommand(command)
// Resolve deny patterns: per-agent overrides from context, fallback to all defaults.
denyOverrides := store.ShellDenyGroupsFromContext(ctx)
// Resolve deny patterns: merge per-agent context overrides with global
// config (per-key agent precedence), fallback to all registry defaults.
denyOverrides := t.effectiveDenyGroups(ctx)
groupPatterns := ResolveDenyPatterns(denyOverrides)
// Also resolve package_install patterns separately for approval routing.
@@ -0,0 +1,100 @@
package tools
import (
"context"
"testing"
"github.com/nextlevelbuilder/goclaw/internal/store"
)
// TestExecToolEffectiveDenyGroups_GlobalOnly: with an empty agent context,
// effectiveDenyGroups must return the global toggles set via SetGlobalShellDenyGroups.
func TestExecToolEffectiveDenyGroups_GlobalOnly(t *testing.T) {
tool := NewExecTool("/tmp", false)
tool.SetGlobalShellDenyGroups(map[string]bool{"package_install": false})
got := tool.effectiveDenyGroups(context.Background())
if v, ok := got["package_install"]; !ok || v != false {
t.Fatalf("expected global package_install=false in effective map, got %v", got)
}
}
// TestExecToolEffectiveDenyGroups_AgentOverridesGlobal: per-agent context
// override wins per-key over global; non-overridden global keys must remain.
func TestExecToolEffectiveDenyGroups_AgentOverridesGlobal(t *testing.T) {
tool := NewExecTool("/tmp", false)
tool.SetGlobalShellDenyGroups(map[string]bool{
"package_install": false,
"env_dump": false,
})
ctx := store.WithShellDenyGroups(context.Background(), map[string]bool{"package_install": true})
got := tool.effectiveDenyGroups(ctx)
if v, ok := got["package_install"]; !ok || v != true {
t.Errorf("expected agent override package_install=true, got %v (ok=%v)", v, ok)
}
if v, ok := got["env_dump"]; !ok || v != false {
t.Errorf("expected global env_dump=false preserved, got %v (ok=%v)", v, ok)
}
}
// TestExecToolEffectiveDenyGroups_NilGlobalReturnsAgent: when no global
// is configured, return the agent map (preserving existing per-agent semantics).
func TestExecToolEffectiveDenyGroups_NilGlobalReturnsAgent(t *testing.T) {
tool := NewExecTool("/tmp", false)
agent := map[string]bool{"foo": true}
ctx := store.WithShellDenyGroups(context.Background(), agent)
got := tool.effectiveDenyGroups(ctx)
if v, ok := got["foo"]; !ok || v != true {
t.Fatalf("expected agent map returned when global empty, got %v", got)
}
}
// TestExecToolEffectiveDenyGroups_EmptyAgentReturnsGlobal: when agent
// context has no overrides, return the global map.
func TestExecToolEffectiveDenyGroups_EmptyAgentReturnsGlobal(t *testing.T) {
tool := NewExecTool("/tmp", false)
tool.SetGlobalShellDenyGroups(map[string]bool{"foo": true})
got := tool.effectiveDenyGroups(context.Background())
if v, ok := got["foo"]; !ok || v != true {
t.Fatalf("expected global map returned when agent empty, got %v", got)
}
}
// TestExecToolSetGlobalShellDenyGroups_DefensiveCopy: mutating the caller's
// map after SetGlobalShellDenyGroups must not affect the tool's internal state.
func TestExecToolSetGlobalShellDenyGroups_DefensiveCopy(t *testing.T) {
tool := NewExecTool("/tmp", false)
src := map[string]bool{"package_install": true}
tool.SetGlobalShellDenyGroups(src)
// Mutate the caller's map AFTER passing it in.
src["package_install"] = false
src["env_dump"] = true
got := tool.effectiveDenyGroups(context.Background())
if v := got["package_install"]; v != true {
t.Errorf("expected internal copy to be insulated from caller mutation; got package_install=%v", v)
}
if _, ok := got["env_dump"]; ok {
t.Errorf("expected internal copy to be insulated from new caller-side keys; got %v", got)
}
}
// TestExecToolSetGlobalShellDenyGroups_EmptyClears: passing an empty map
// must clear the internal state, not retain a stale copy.
func TestExecToolSetGlobalShellDenyGroups_EmptyClears(t *testing.T) {
tool := NewExecTool("/tmp", false)
tool.SetGlobalShellDenyGroups(map[string]bool{"foo": true})
tool.SetGlobalShellDenyGroups(map[string]bool{})
got := tool.effectiveDenyGroups(context.Background())
if len(got) != 0 {
t.Fatalf("expected cleared global to yield empty effective map, got %v", got)
}
}