From 0b64808ef540259c106ae2278192f68f883da58d Mon Sep 17 00:00:00 2001 From: Levi Neely Date: Tue, 21 Apr 2026 13:38:02 +0200 Subject: [PATCH] pkg/tools: add contract tests, remove Close from Server/Dispatcher - Add contract tests for tools.Server and tools.Dispatcher interfaces (tools_test.go), covering all exported behavior including error paths. pkg/tools coverage: 0% -> 100%. - Add contract test for execute.Server (ListTools, CallTool), Decl factory test, and executeElevated early-exit test. execute coverage: 68% -> 71%. - Remove Close() from Server and Dispatcher interfaces and all implementations/callers. These existed for MCP server cleanup which has been removed. --- pkg/agent/core.go | 3 - pkg/agent/core_test.go | 1 - pkg/tools/execute/execute_test.go | 62 +++++++++++++ pkg/tools/execute/server.go | 3 - pkg/tools/tools.go | 9 -- pkg/tools/tools_test.go | 149 ++++++++++++++++++++++++++++++ 6 files changed, 211 insertions(+), 16 deletions(-) create mode 100644 pkg/tools/tools_test.go diff --git a/pkg/agent/core.go b/pkg/agent/core.go index 22cbb00..ffed903 100644 --- a/pkg/agent/core.go +++ b/pkg/agent/core.go @@ -971,9 +971,6 @@ func (s *agentCore) handleCommand(ctx context.Context, input string, handler Eve handler(infoEvent(fmt.Sprintf("error: agent %q: %v", name, err))) return } - if s.dispatcher != nil { - s.dispatcher.Close() - } d := s.newDispatcher() env := BuildAgentEnv(cfg, d, s.cwd) s.dispatcher = env.dispatcher diff --git a/pkg/agent/core_test.go b/pkg/agent/core_test.go index 73c1a89..5306c06 100644 --- a/pkg/agent/core_test.go +++ b/pkg/agent/core_test.go @@ -2067,7 +2067,6 @@ func (m *mockEnvServer) ListTools() ([]tools.ToolInfo, error) func (m *mockEnvServer) CallTool(_ context.Context, _ string, _ json.RawMessage) (json.RawMessage, error) { return nil, fmt.Errorf("not implemented") } -func (m *mockEnvServer) Close() {} func (m *mockEnvServer) SetEnv(k, v string) { m.mu.Lock() defer m.mu.Unlock() diff --git a/pkg/tools/execute/execute_test.go b/pkg/tools/execute/execute_test.go index 3f8b7a5..c874062 100644 --- a/pkg/tools/execute/execute_test.go +++ b/pkg/tools/execute/execute_test.go @@ -385,3 +385,65 @@ func TestSetCWD(t *testing.T) { t.Errorf("pwd output %q doesn't contain expected dir %q", out, dir) } } + +// ---- Server contract (ListTools / CallTool / Close) ---- + +func TestServerContract(t *testing.T) { + s := newServer(t) + tools, err := s.ListTools() + if err != nil { + t.Fatalf("ListTools: %v", err) + } + if len(tools) == 0 { + t.Fatal("ListTools returned no tools") + } + for _, ti := range tools { + if ti.Name == "" { + t.Error("tool has empty Name") + } + if ti.Description == "" { + t.Errorf("tool %q has empty Description", ti.Name) + } + if len(ti.InputSchema) == 0 { + t.Errorf("tool %q has empty InputSchema", ti.Name) + } + } + // CallTool success path + args, _ := json.Marshal(map[string]any{"steps": []map[string]any{{"code": "echo contract"}}}) + res, err := s.CallTool(context.Background(), "execute_code", args) + if err != nil { + t.Fatalf("CallTool: %v", err) + } + if !strings.Contains(string(res), "contract") { + t.Errorf("CallTool result missing expected output: %s", res) + } + // CallTool error path (unknown tool) + res, err = s.CallTool(context.Background(), "bogus", json.RawMessage(`{}`)) + if err != nil { + t.Fatalf("CallTool error path should marshal, not return error: %v", err) + } + if !strings.Contains(string(res), "isError") { + t.Errorf("CallTool error path should contain isError: %s", res) + } +} + +// ---- Decl factory ---- + +func TestDecl(t *testing.T) { + factory := Decl(t.TempDir()) + srv := factory() + if srv == nil { + t.Fatal("Decl factory returned nil") + } +} + +// ---- executeElevated (plugin not found) ---- + +func TestExecuteElevatedNoPlugin(t *testing.T) { + s := newServer(t) + t.Setenv("OLLIE_PLUGINS_PATH", t.TempDir()) // empty dir, no elevate script + _, err := s.executeElevated(context.Background(), "echo hi", t.TempDir(), 10) + if err == nil || !strings.Contains(err.Error(), "elevation not available") { + t.Errorf("expected 'elevation not available' error, got %v", err) + } +} diff --git a/pkg/tools/execute/server.go b/pkg/tools/execute/server.go index 628561c..b06d854 100644 --- a/pkg/tools/execute/server.go +++ b/pkg/tools/execute/server.go @@ -388,7 +388,4 @@ func prependOlliePath(env []string) []string { return result } -// Close implements tools.Server (no-op). -func (e *Server) Close() {} - var _ tools.Server = (*Server)(nil) // compile-time interface check diff --git a/pkg/tools/tools.go b/pkg/tools/tools.go index 7a9b6ff..d1c0485 100644 --- a/pkg/tools/tools.go +++ b/pkg/tools/tools.go @@ -20,7 +20,6 @@ type ToolInfo struct { type Server interface { ListTools() ([]ToolInfo, error) CallTool(ctx context.Context, tool string, args json.RawMessage) (json.RawMessage, error) - Close() } // Dispatcher routes tool calls to the server that owns them. @@ -29,7 +28,6 @@ type Dispatcher interface { GetServer(name string) (Server, bool) ListTools() ([]ToolInfo, error) Dispatch(ctx context.Context, server, tool string, args json.RawMessage) (json.RawMessage, error) - Close() } // dispatcher is the default Dispatcher backed by registered Server instances. @@ -66,13 +64,6 @@ func (d *dispatcher) GetServer(name string) (Server, bool) { return s, ok } -// Close shuts down all registered servers. -func (d *dispatcher) Close() { - for _, s := range d.servers { - s.Close() - } -} - // ListTools returns all tools advertised by all registered servers. func (d *dispatcher) ListTools() ([]ToolInfo, error) { var all []ToolInfo diff --git a/pkg/tools/tools_test.go b/pkg/tools/tools_test.go new file mode 100644 index 0000000..cadaf9e --- /dev/null +++ b/pkg/tools/tools_test.go @@ -0,0 +1,149 @@ +package tools_test + +import ( + "context" + "encoding/json" + "fmt" + "testing" + + "ollie/pkg/tools" +) + +// stubServer is a minimal Server used to verify the contract. +type stubServer struct { + name string + tools []tools.ToolInfo +} + +func (s *stubServer) ListTools() ([]tools.ToolInfo, error) { return s.tools, nil } +func (s *stubServer) CallTool(_ context.Context, tool string, _ json.RawMessage) (json.RawMessage, error) { + return json.RawMessage(`{"tool":"` + tool + `"}`), nil +} + +func newStub(name string, toolNames ...string) *stubServer { + var ti []tools.ToolInfo + for _, n := range toolNames { + ti = append(ti, tools.ToolInfo{Name: n, Description: n + " desc"}) + } + return &stubServer{name: name, tools: ti} +} + +// checkServerContract verifies Server invariants. +func checkServerContract(t *testing.T, s tools.Server) { + t.Helper() + tl, err := s.ListTools() + if err != nil { + t.Fatalf("ListTools: %v", err) + } + if tl == nil { + t.Fatal("ListTools returned nil") + } + for _, ti := range tl { + res, err := s.CallTool(context.Background(), ti.Name, json.RawMessage(`{}`)) + if err != nil { + t.Errorf("CallTool(%q): %v", ti.Name, err) + } + if len(res) == 0 { + t.Errorf("CallTool(%q) returned empty result", ti.Name) + } + } +} + +// checkDispatcherContract verifies Dispatcher invariants. +func checkDispatcherContract(t *testing.T, d tools.Dispatcher, servers map[string]*stubServer) { + t.Helper() + + for name, s := range servers { + d.AddServer(name, s) + } + + // GetServer round-trip + for name := range servers { + s, ok := d.GetServer(name) + if !ok || s == nil { + t.Errorf("GetServer(%q) not found after AddServer", name) + } + } + if _, ok := d.GetServer("nonexistent"); ok { + t.Error("GetServer returned true for unregistered server") + } + + // ListTools aggregates all servers + all, err := d.ListTools() + if err != nil { + t.Fatalf("ListTools: %v", err) + } + var wantCount int + for _, s := range servers { + wantCount += len(s.tools) + } + if len(all) != wantCount { + t.Errorf("ListTools returned %d tools, want %d", len(all), wantCount) + } + for _, ti := range all { + if ti.Server == "" { + t.Errorf("tool %q has empty Server field", ti.Name) + } + } + + // Dispatch routes to correct server + for name, s := range servers { + for _, ti := range s.tools { + res, err := d.Dispatch(context.Background(), name, ti.Name, json.RawMessage(`{}`)) + if err != nil { + t.Errorf("Dispatch(%q, %q): %v", name, ti.Name, err) + } + if len(res) == 0 { + t.Errorf("Dispatch(%q, %q) returned empty", name, ti.Name) + } + } + } + + // Dispatch to unknown server must error + _, err = d.Dispatch(context.Background(), "nonexistent", "tool", json.RawMessage(`{}`)) + if err == nil { + t.Error("Dispatch to unknown server should error") + } +} + +func TestStubServerContract(t *testing.T) { + checkServerContract(t, newStub("s", "a", "b")) +} + +func TestDispatcherContract(t *testing.T) { + servers := map[string]*stubServer{ + "alpha": newStub("alpha", "tool1", "tool2"), + "beta": newStub("beta", "tool3"), + } + checkDispatcherContract(t, tools.NewDispatcher(), servers) +} + +// failServer is a Server whose ListTools always errors. +type failServer struct{ stubServer } + +func (f *failServer) ListTools() ([]tools.ToolInfo, error) { + return nil, fmt.Errorf("boom") +} + +func TestDispatcherListToolsError(t *testing.T) { + d := tools.NewDispatcher() + d.AddServer("bad", &failServer{}) + _, err := d.ListTools() + if err == nil { + t.Error("expected error from failing server") + } +} + +func TestNewDispatcherFunc(t *testing.T) { + factory := tools.NewDispatcherFunc(map[string]func() tools.Server{ + "s1": func() tools.Server { return newStub("s1", "t1") }, + }) + d := factory() + tl, err := d.ListTools() + if err != nil { + t.Fatalf("ListTools: %v", err) + } + if len(tl) != 1 { + t.Errorf("got %d tools, want 1", len(tl)) + } +}