From 1207e26a07b08cb1f5e34b42d2cdc31ed62906bb Mon Sep 17 00:00:00 2001 From: Levi Neely <141506390+lneely@users.noreply.github.com> Date: Sun, 17 May 2026 15:21:26 +0200 Subject: [PATCH] remove trustedTools and Confirm callback; trust is implicit via allowExecutors --- pkg/agent/core.go | 7 ------ pkg/agent/core_test.go | 24 -------------------- pkg/config/config.go | 1 - pkg/config/config_test.go | 4 ---- pkg/tools/execute/code.go | 35 ----------------------------- pkg/tools/execute/execute_test.go | 37 +------------------------------ pkg/tools/execute/server.go | 34 ---------------------------- pkg/tools/tools.go | 6 ----- 8 files changed, 1 insertion(+), 147 deletions(-) diff --git a/pkg/agent/core.go b/pkg/agent/core.go index 2eb8d4c..f698694 100644 --- a/pkg/agent/core.go +++ b/pkg/agent/core.go @@ -99,13 +99,6 @@ func BuildAgentEnv(cfg *config.Config, d tools.Dispatcher, cwd string) AgentEnv Verbosity: cfg.Verbosity, } maxSteps = cfg.MaxSteps - if len(cfg.TrustedTools) > 0 { - if srv, ok := d.GetServer("execute"); ok { - if ts, ok := srv.(tools.TrustedToolsSetter); ok { - ts.SetTrustedTools(cfg.TrustedTools) - } - } - } if len(cfg.AllowExecutors) > 0 || len(cfg.AllowTools) > 0 { if srv, ok := d.GetServer("execute"); ok { if rs, ok := srv.(tools.ToolRestrictionSetter); ok { diff --git a/pkg/agent/core_test.go b/pkg/agent/core_test.go index 0191afc..984306b 100644 --- a/pkg/agent/core_test.go +++ b/pkg/agent/core_test.go @@ -2315,19 +2315,6 @@ func (m *mockDispatcher) Dispatch(ctx context.Context, server, tool string, args return nil, fmt.Errorf("not implemented") } -type mockTrustedSetter struct { - tools.Server - trusted []string -} - -func (m *mockTrustedSetter) SetTrustedTools(t []string) { m.trusted = t } -func (m *mockTrustedSetter) ListTools() ([]tools.ToolInfo, error) { - return nil, nil -} -func (m *mockTrustedSetter) CallTool(ctx context.Context, tool string, args json.RawMessage) (json.RawMessage, error) { - return nil, nil -} - func TestBuildAgentEnv_ListToolsError(t *testing.T) { setupCfgDir(t) d := &mockDispatcher{listErr: fmt.Errorf("boom")} @@ -2361,17 +2348,6 @@ func TestBuildAgentEnv_ToolsDisabled(t *testing.T) { } } -func TestBuildAgentEnv_TrustedTools(t *testing.T) { - setupCfgDir(t) - setter := &mockTrustedSetter{} - d := &mockDispatcher{servers: map[string]tools.Server{"execute": setter}} - cfg := &config.Config{TrustedTools: []string{"bash"}} - BuildAgentEnv(cfg, d, t.TempDir()) - if len(setter.trusted) != 1 || setter.trusted[0] != "bash" { - t.Errorf("trusted = %v", setter.trusted) - } -} - func TestBuildAgentEnv_PromptOnly(t *testing.T) { setupCfgDir(t) d := tools.NewDispatcher() diff --git a/pkg/config/config.go b/pkg/config/config.go index 145ebdc..69a28d9 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -64,7 +64,6 @@ type Config struct { Backend string `json:"backend,omitempty"` Model string `json:"model,omitempty"` Tools *bool `json:"tools,omitempty"` - TrustedTools []string `json:"trustedTools,omitempty"` AllowExecutors []string `json:"allowExecutors,omitempty"` AllowTools []string `json:"allowTools,omitempty"` MaxTokens int `json:"maxTokens,omitempty"` diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 3a0b449..7ad8303 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -93,7 +93,6 @@ func TestPromptInvalid(t *testing.T) { func TestLoadAllFields(t *testing.T) { cfg, err := Load(strings.NewReader(`{ "prompt": "be helpful", - "trustedTools": ["execute_code"], "maxTokens": 4096, "temperature": 0.7, "frequencyPenalty": 0.5, @@ -105,9 +104,6 @@ func TestLoadAllFields(t *testing.T) { if cfg.Prompt.IsExec || len(cfg.Prompt.Value) != 1 || cfg.Prompt.Value[0] != "be helpful" { t.Errorf("Prompt = %v", cfg.Prompt) } - if len(cfg.TrustedTools) != 1 || cfg.TrustedTools[0] != "execute_code" { - t.Errorf("TrustedTools = %v", cfg.TrustedTools) - } if cfg.MaxTokens != 4096 { t.Errorf("MaxTokens = %d", cfg.MaxTokens) } diff --git a/pkg/tools/execute/code.go b/pkg/tools/execute/code.go index 9987e5f..0b729c9 100644 --- a/pkg/tools/execute/code.go +++ b/pkg/tools/execute/code.go @@ -248,11 +248,6 @@ func (e *Server) dispatchSteps(ctx context.Context, steps []CodeStep, timeout in } } - label := buildExecLabel(steps) - if !e.allowed("execute_code", label) { - return "", fmt.Errorf("execute_code: denied by user") - } - // Single step: run directly. if len(steps) == 1 && len(steps[0].Parallel) == 0 { if steps[0].Elevated { @@ -324,11 +319,6 @@ func (e *Server) dispatchPipe(ctx context.Context, pipe []CodeStep, timeout int, } } - label := buildExecLabel(pipe) - if !e.allowed("execute_code", label) { - return "", fmt.Errorf("execute_code: denied by user") - } - var input string for i, stage := range pipe { out, err := e.runStage(ctx, i, stage, timeout, sandboxName, input) @@ -340,31 +330,6 @@ func (e *Server) dispatchPipe(ctx context.Context, pipe []CodeStep, timeout int, return input, nil } -func buildExecLabel(stages []CodeStep) string { - stageLabel := func(s CodeStep) string { - prefix := "" - if s.Elevated { - prefix = "[elevated] " - } - if len(s.Parallel) > 0 { - return fmt.Sprintf("%sparallel(%d)", prefix, len(s.Parallel)) - } - if s.Tool != "" { - return fmt.Sprintf("%stool:%s %s", prefix, s.Tool, strings.Join(s.Args, " ")) - } - return prefix + s.Code - } - if len(stages) == 1 { - return fmt.Sprintf("execute_code: %s", stageLabel(stages[0])) - } - var sb strings.Builder - fmt.Fprintf(&sb, "execute_code: %d stages", len(stages)) - for i, s := range stages { - fmt.Fprintf(&sb, "\n [%d]: %s", i, stageLabel(s)) - } - return sb.String() -} - // enforceStrict rejects any step that uses inline code rather than a named tool. func enforceStrict(stages []CodeStep) error { for i, s := range stages { diff --git a/pkg/tools/execute/execute_test.go b/pkg/tools/execute/execute_test.go index 710502c..756e9ae 100644 --- a/pkg/tools/execute/execute_test.go +++ b/pkg/tools/execute/execute_test.go @@ -20,12 +20,10 @@ func TestMain(m *testing.M) { os.Exit(m.Run()) } -// newServer returns a Server with Confirm always returning true and Yolo=true -// so tests run without landrun installed in CI. +// newServer returns a Server with Yolo=true so tests run without landrun in CI. func newServer(t *testing.T) *Server { t.Helper() s := New(t.TempDir()) - s.Confirm = func(string) bool { return true } s.Yolo = true // skip landrun in unit tests return s } @@ -193,31 +191,6 @@ func TestValidateCodeSafe(t *testing.T) { } } -// ---- allowed ---- - -func TestAllowedWithConfirm(t *testing.T) { - s := New(t.TempDir()) - s.Confirm = func(string) bool { return true } - if !s.allowed("execute_code", "echo hi") { - t.Error("expected allowed=true when Confirm returns true") - } -} - -func TestAllowedDeniedNoConfirm(t *testing.T) { - s := New(t.TempDir()) - if s.allowed("execute_code", "echo hi") { - t.Error("expected allowed=false when Confirm is nil") - } -} - -func TestAllowedTrustedTool(t *testing.T) { - s := New(t.TempDir()) - s.SetTrustedTools([]string{"execute_code"}) - if !s.allowed("execute_code", "anything") { - t.Error("expected trusted tool to be allowed without Confirm") - } -} - // ---- Execute (integration, requires bash) ---- func TestExecuteSimpleBash(t *testing.T) { @@ -284,14 +257,6 @@ func TestDispatchNoSteps(t *testing.T) { } } -func TestDispatchDenied(t *testing.T) { - s := New(t.TempDir()) // no Confirm, not trusted → denied - _, err := callCode(t, s, []map[string]any{{"code": "echo hi"}}) - if err == nil || !strings.Contains(err.Error(), "denied") { - t.Errorf("expected denied error, got %v", err) - } -} - func TestDispatchSingleStep(t *testing.T) { s := newServer(t) out, err := callCode(t, s, []map[string]any{{"code": "echo single"}}) diff --git a/pkg/tools/execute/server.go b/pkg/tools/execute/server.go index a2c8dc6..6332edf 100644 --- a/pkg/tools/execute/server.go +++ b/pkg/tools/execute/server.go @@ -28,14 +28,6 @@ const ( // Server runs code in a sandboxed environment. type Server struct { - // Confirm is an optional function called before executing sensitive operations. - // Returns true to allow, false to deny. Trusted tools bypass this check. - Confirm func(string) bool - - // trusted is the set of tool names that bypass Confirm. - trustedMu sync.RWMutex - trusted map[string]bool - // cwd is the working directory for sandboxed commands. If empty, // the process working directory is used. wdMu sync.RWMutex @@ -330,16 +322,6 @@ func (e *Server) SetCWD(dir string) { e.wdMu.Unlock() } -// SetTrustedTools marks the given tool names as trusted; they bypass Confirm. -func (e *Server) SetTrustedTools(tools []string) { - e.trustedMu.Lock() - e.trusted = make(map[string]bool, len(tools)) - for _, t := range tools { - e.trusted[t] = true - } - e.trustedMu.Unlock() -} - // SetAllowExecutors restricts which executors are available. func (e *Server) SetAllowExecutors(names []string) { if len(names) > 0 { @@ -364,22 +346,6 @@ func (e *Server) SetAllowTools(names []string) { } } -// allowed returns true if the tool call should proceed. -// Trusted tools are always allowed. Untrusted tools are passed to Confirm; -// if Confirm is nil, untrusted tools are denied. -func (e *Server) allowed(tool, detail string) bool { - e.trustedMu.RLock() - trusted := e.trusted[tool] - e.trustedMu.RUnlock() - if trusted { - return true - } - if e.Confirm == nil { - return false // no confirm fn and not trusted: deny by default - } - return e.Confirm(detail) -} - // SetLockDir sets the directory used for advisory flock files during parallel // step dispatch. Must be called before the Server handles concurrent requests. func (e *Server) SetLockDir(dir string) { e.lockDir = dir } diff --git a/pkg/tools/tools.go b/pkg/tools/tools.go index aaf308d..7b58ec2 100644 --- a/pkg/tools/tools.go +++ b/pkg/tools/tools.go @@ -101,12 +101,6 @@ type EnvSetter interface { SetEnv(key, value string) } -// TrustedToolsSetter is implemented by tool servers that support a trusted-tool -// list. Tools in the list bypass the Confirm callback. -type TrustedToolsSetter interface { - SetTrustedTools(tools []string) -} - // LockDirSetter is implemented by tool servers that use advisory flock files // for parallel-step coordination. SetLockDir must be called once at session init. type LockDirSetter interface {