From 97da480d20c2dd1af23ae9cbb649dcb664996282 Mon Sep 17 00:00:00 2001 From: Ed Zynda Date: Wed, 13 May 2026 20:31:43 +0300 Subject: [PATCH] fix(sdk): stop leaking fantasy types through pkg/kit.AgentConfig (#30) Replace the alias-based AgentConfig and handler types with SDK-owned structs and function types. CoreTools / ExtraTools / ToolWrapper now accept []kit.Tool, and the handler types (ToolCallHandler, ToolExecutionHandler, ToolResultHandler, ResponseHandler, StreamingResponseHandler, ToolCallContentHandler) plus SpinnerFunc are declared in pkg/kit/ with signatures that reference only SDK types. Consumers no longer need to import charm.land/fantasy to populate an AgentConfig or assign a handler. go doc pkg/kit AgentConfig output no longer mentions fantasy.*. - Add unexported (*AgentConfig).toInternal() to convert at the SDK boundary; Tool is still an alias for the underlying tool type, so slice and function fields convert without allocation. - Add agent_config_internal_test.go covering nil receiver, scalar fields, tool slices, ToolWrapper invocation, OnMCPServerLoaded, and auth/token-factory wiring. - Add types_test.go cases that populate AgentConfig and SpinnerFunc without importing fantasy -- the file compiling is the regression proof for the leak. - Update pkg/kit/README.md Re-exported Types section to record that AgentConfig and the handler types are now Kit-owned. Fixes #30 --- pkg/kit/README.md | 17 ++- pkg/kit/agent_config_internal_test.go | 144 ++++++++++++++++++++++++++ pkg/kit/kit.go | 2 +- pkg/kit/types.go | 129 +++++++++++++++++++---- pkg/kit/types_test.go | 96 +++++++++++++++++ 5 files changed, 367 insertions(+), 21 deletions(-) create mode 100644 pkg/kit/agent_config_internal_test.go diff --git a/pkg/kit/README.md b/pkg/kit/README.md index 5e06ccfb..c8ba204a 100644 --- a/pkg/kit/README.md +++ b/pkg/kit/README.md @@ -243,7 +243,7 @@ host.ClearSession() ## Re-exported Types -The SDK re-exports types so you don't need direct internal imports: +The SDK re-exports message/session/MCP types so you don't need direct internal imports. Agent-configuration types are Kit-owned (not aliases) and use only SDK types in their signatures, so consumers never need to import the underlying LLM-provider package. ```go // Message types @@ -251,13 +251,26 @@ kit.Message, kit.MessageRole, kit.ContentPart kit.TextContent, kit.ReasoningContent, kit.ToolCall, kit.ToolResult, kit.Finish kit.RoleUser, kit.RoleAssistant, kit.RoleTool, kit.RoleSystem -// LLM types — concrete Kit-owned structs, no external library dependency +// LLM types — Kit-owned `LLM*` aliases over the underlying provider types, +// so consumers never import the provider package directly kit.LLMMessage // {Role LLMMessageRole, Content string} kit.LLMMessageRole // "user" | "assistant" | "system" | "tool" kit.LLMUsage // {InputTokens, OutputTokens, TotalTokens, ...} kit.LLMResponse // {Content, FinishReason, Usage} kit.LLMFilePart // {Filename, Data []byte, MediaType} +// Agent configuration — concrete Kit-owned structs and function types. +// All fields use SDK types (e.g. `[]kit.Tool`), so consumers can construct +// these without importing any LLM-provider package. +kit.AgentConfig // Lower-level agent config — prefer Options unless you need direct control +kit.ToolCallHandler // func(toolCallID, toolName, toolArgs string) +kit.ToolExecutionHandler // func(toolCallID, toolName, toolArgs string, isStarting bool) +kit.ToolResultHandler // func(toolCallID, toolName, toolArgs, result, metadata string, isError bool) +kit.ResponseHandler // func(content string) +kit.StreamingResponseHandler // func(content string) +kit.ToolCallContentHandler // func(content string) +kit.SpinnerFunc // func(fn func() error) error + // MCP OAuth types kit.MCPServer // *server.MCPServer for in-process MCP transport kit.MCPServerConfig // Configuration for an MCP server (stdio, SSE, or in-process) diff --git a/pkg/kit/agent_config_internal_test.go b/pkg/kit/agent_config_internal_test.go new file mode 100644 index 00000000..ef9d5b4e --- /dev/null +++ b/pkg/kit/agent_config_internal_test.go @@ -0,0 +1,144 @@ +package kit + +import ( + "context" + "errors" + "testing" + + "github.com/mark3labs/kit/internal/agent" +) + +// TestAgentConfigToInternal verifies that the SDK-side AgentConfig converts +// faithfully to the internal agent.AgentConfig representation, preserving +// every field consumed by the internal agent layer. +// +// Regression test for https://github.com/mark3labs/kit/issues/30. +func TestAgentConfigToInternal(t *testing.T) { + t.Run("nil receiver returns nil", func(t *testing.T) { + var c *AgentConfig + if got := c.toInternal(); got != nil { + t.Errorf("nil.toInternal() = %v, want nil", got) + } + }) + + t.Run("scalar fields round-trip", func(t *testing.T) { + c := &AgentConfig{ + SystemPrompt: "sys", + MaxSteps: 7, + StreamingEnabled: true, + DisableCoreTools: true, + } + got := c.toInternal() + if got == nil { + t.Fatal("toInternal() = nil") + } + if got.SystemPrompt != "sys" { + t.Errorf("SystemPrompt = %q, want %q", got.SystemPrompt, "sys") + } + if got.MaxSteps != 7 { + t.Errorf("MaxSteps = %d, want 7", got.MaxSteps) + } + if !got.StreamingEnabled { + t.Error("StreamingEnabled = false, want true") + } + if !got.DisableCoreTools { + t.Error("DisableCoreTools = false, want true") + } + }) + + t.Run("tool slices propagate without conversion", func(t *testing.T) { + // Tool is a type alias for the underlying LLM-tool type, so the + // SDK []Tool and internal []fantasy.AgentTool slices share the + // same backing array after conversion. + tool := NewTool[struct{}]("noop", "noop", nil) + c := &AgentConfig{ + CoreTools: []Tool{tool}, + ExtraTools: []Tool{tool, tool}, + } + got := c.toInternal() + if len(got.CoreTools) != 1 { + t.Errorf("CoreTools len = %d, want 1", len(got.CoreTools)) + } + if len(got.ExtraTools) != 2 { + t.Errorf("ExtraTools len = %d, want 2", len(got.ExtraTools)) + } + }) + + t.Run("tool wrapper is invoked through internal config", func(t *testing.T) { + called := false + c := &AgentConfig{ + ToolWrapper: func(in []Tool) []Tool { + called = true + return in + }, + } + got := c.toInternal() + if got.ToolWrapper == nil { + t.Fatal("internal ToolWrapper is nil") + } + _ = got.ToolWrapper(nil) + if !called { + t.Error("SDK ToolWrapper was not invoked through the internal config") + } + }) + + t.Run("OnMCPServerLoaded propagates", func(t *testing.T) { + var captured string + wantErr := errors.New("boom") + c := &AgentConfig{ + OnMCPServerLoaded: func(name string, _ int, _ error) { + captured = name + }, + } + got := c.toInternal() + got.OnMCPServerLoaded("svr", 3, wantErr) + if captured != "svr" { + t.Errorf("OnMCPServerLoaded captured = %q, want %q", captured, "svr") + } + }) + + t.Run("auth and token store factories are wired", func(t *testing.T) { + auth := &fakeAuthHandler{} + tokenCalls := 0 + var tokenServer string + factory := MCPTokenStoreFactory(func(server string) (MCPTokenStore, error) { + tokenCalls++ + tokenServer = server + return nil, nil + }) + c := &AgentConfig{ + AuthHandler: auth, + TokenStoreFactory: factory, + } + got := c.toInternal() + if got.AuthHandler == nil { + t.Fatal("internal AuthHandler is nil") + } + if got.TokenStoreFactory == nil { + t.Fatal("internal TokenStoreFactory is nil") + } + _, _ = got.TokenStoreFactory("https://example.test") + if tokenCalls != 1 { + t.Errorf("token factory call count = %d, want 1", tokenCalls) + } + if tokenServer != "https://example.test" { + t.Errorf("token factory server arg = %q", tokenServer) + } + if got.AuthHandler.RedirectURI() != "redirect" { + t.Errorf("RedirectURI = %q, want %q", got.AuthHandler.RedirectURI(), "redirect") + } + }) + + // Compile-time check that the internal type is what we expect. + //nolint:staticcheck // QF1011: explicit type asserts the conversion target. + var _ *agent.AgentConfig = (&AgentConfig{}).toInternal() +} + +// fakeAuthHandler implements both kit.MCPAuthHandler and the structurally +// identical tools.MCPAuthHandler used by the internal layer. +type fakeAuthHandler struct{} + +func (f *fakeAuthHandler) RedirectURI() string { return "redirect" } +func (f *fakeAuthHandler) HandleAuth(_ context.Context, _ string, _ string) (string, error) { + return "", nil +} diff --git a/pkg/kit/kit.go b/pkg/kit/kit.go index b42f469f..e79048a4 100644 --- a/pkg/kit/kit.go +++ b/pkg/kit/kit.go @@ -1489,7 +1489,7 @@ func New(ctx context.Context, opts *Options) (*Kit, error) { if opts.CLI != nil { setupOpts.ShowSpinner = opts.CLI.ShowSpinner - setupOpts.SpinnerFunc = opts.CLI.SpinnerFunc + setupOpts.SpinnerFunc = agent.SpinnerFunc(opts.CLI.SpinnerFunc) setupOpts.UseBufferedLogger = opts.CLI.UseBufferedLogger if opts.CLI.ProgressReaderFunc != nil { providerConfig.ProgressReaderFunc = opts.CLI.ProgressReaderFunc diff --git a/pkg/kit/types.go b/pkg/kit/types.go index 952038fe..a92346c5 100644 --- a/pkg/kit/types.go +++ b/pkg/kit/types.go @@ -11,6 +11,7 @@ import ( "github.com/mark3labs/kit/internal/message" "github.com/mark3labs/kit/internal/models" "github.com/mark3labs/kit/internal/session" + "github.com/mark3labs/kit/internal/tools" "github.com/mark3labs/mcp-go/client/transport" "github.com/mark3labs/mcp-go/server" ) @@ -75,25 +76,117 @@ type Config = config.Config // local (stdio) and remote (StreamableHTTP/SSE) server types. type MCPServerConfig = config.MCPServerConfig -// ==== Agent Types (internal/agent/) ==== +// ==== Agent Types ==== -// AgentConfig holds configuration options for creating a new Agent. -type AgentConfig = agent.AgentConfig +// AgentConfig holds configuration options for constructing an agent at the +// SDK boundary. All fields use SDK-owned types, so consumers can populate +// this struct without importing any underlying LLM-provider package. +// +// For most use cases, prefer the high-level [New] entry point with +// [Options]. AgentConfig is exposed for advanced consumers that need +// direct access to the lower-level agent configuration shape. +type AgentConfig struct { + // ModelConfig holds the LLM provider configuration. A nil value means + // that the default provider/model resolution will be used. + ModelConfig *ProviderConfig -type ( - // ToolCallHandler is a function type for handling tool calls as they happen. - ToolCallHandler = agent.ToolCallHandler - // ToolExecutionHandler is a function type for handling tool execution start/end events. - ToolExecutionHandler = agent.ToolExecutionHandler - // ToolResultHandler is a function type for handling tool results. - ToolResultHandler = agent.ToolResultHandler - // ResponseHandler is a function type for handling LLM responses. - ResponseHandler = agent.ResponseHandler - // StreamingResponseHandler is a function type for handling streaming LLM responses. - StreamingResponseHandler = agent.StreamingResponseHandler - // ToolCallContentHandler is a function type for handling content that accompanies tool calls. - ToolCallContentHandler = agent.ToolCallContentHandler -) + // MCPConfig describes any MCP servers whose tools should be loaded + // alongside core tools. + MCPConfig *Config + + // SystemPrompt is the system prompt sent to the LLM. + SystemPrompt string + + // MaxSteps caps the number of LLM iterations per turn. A value of + // zero means no cap is applied at this layer. + MaxSteps int + + // StreamingEnabled controls whether the agent streams responses. + StreamingEnabled bool + + // AuthHandler handles OAuth authorization for remote MCP servers. + // When nil, remote MCP servers requiring OAuth will fail to connect. + AuthHandler MCPAuthHandler + + // TokenStoreFactory, if non-nil, creates a custom token store for each + // remote MCP server's OAuth tokens. When nil, the default file-based + // token store is used. + TokenStoreFactory MCPTokenStoreFactory + + // CoreTools overrides the default core tool set. If empty, [AllTools] + // is used. Provide a custom tool set (e.g. [CodingTools] or tools + // built with a custom WorkDir) to scope agent capabilities. + CoreTools []Tool + + // DisableCoreTools, when true, prevents loading any core tools. + // Combined with empty CoreTools this yields a chat-only agent with + // no built-in tools. + DisableCoreTools bool + + // ExtraTools are additional tools loaded alongside core and MCP tools. + ExtraTools []Tool + + // ToolWrapper, if non-nil, wraps the combined tool list before it is + // handed to the LLM. Used to intercept tool calls or results. + ToolWrapper func([]Tool) []Tool + + // OnMCPServerLoaded, if non-nil, is invoked once for each MCP server + // when its tools have finished loading (or failed). Called from a + // background goroutine. + OnMCPServerLoaded func(serverName string, toolCount int, err error) +} + +// toInternal converts an AgentConfig to its internal representation. +// Slice and function fields convert without allocation because [Tool] +// is a type alias for the underlying LLM-tool type. +func (c *AgentConfig) toInternal() *agent.AgentConfig { + if c == nil { + return nil + } + out := &agent.AgentConfig{ + ModelConfig: c.ModelConfig, + MCPConfig: c.MCPConfig, + SystemPrompt: c.SystemPrompt, + MaxSteps: c.MaxSteps, + StreamingEnabled: c.StreamingEnabled, + CoreTools: c.CoreTools, + DisableCoreTools: c.DisableCoreTools, + ExtraTools: c.ExtraTools, + ToolWrapper: c.ToolWrapper, + OnMCPServerLoaded: c.OnMCPServerLoaded, + } + if c.AuthHandler != nil { + out.AuthHandler = c.AuthHandler + } + if c.TokenStoreFactory != nil { + out.TokenStoreFactory = tools.TokenStoreFactory(c.TokenStoreFactory) + } + return out +} + +// ToolCallHandler is invoked when the LLM produces a tool call. It receives +// the call ID, tool name, and the JSON-encoded input arguments. +type ToolCallHandler func(toolCallID, toolName, toolArgs string) + +// ToolExecutionHandler is invoked at the start and end of tool execution. +// The isStarting flag distinguishes the two phases. +type ToolExecutionHandler func(toolCallID, toolName, toolArgs string, isStarting bool) + +// ToolResultHandler is invoked after a tool finishes executing. The metadata +// parameter carries optional structured data (e.g. file-diff info) from the +// tool execution, JSON-encoded; it may be empty. +type ToolResultHandler func(toolCallID, toolName, toolArgs, result, metadata string, isError bool) + +// ResponseHandler is invoked with the final assistant text for each turn. +type ResponseHandler func(content string) + +// StreamingResponseHandler is invoked with each streamed text delta as it +// arrives from the LLM. +type StreamingResponseHandler func(content string) + +// ToolCallContentHandler is invoked with any assistant text that accompanies +// a tool call within the same step. +type ToolCallContentHandler func(content string) // ==== Provider & Model Types (internal/models/) ==== @@ -126,7 +219,7 @@ type ModelsRegistry = models.ModelsRegistry // SpinnerFunc wraps a function in a loading spinner animation. Used for // Ollama model loading. Signature: func(fn func() error) error. -type SpinnerFunc = agent.SpinnerFunc +type SpinnerFunc func(fn func() error) error // ==== LLM Types ==== // diff --git a/pkg/kit/types_test.go b/pkg/kit/types_test.go index 3b61e4c0..74a6b7fb 100644 --- a/pkg/kit/types_test.go +++ b/pkg/kit/types_test.go @@ -1,6 +1,7 @@ package kit_test import ( + "context" "encoding/json" "testing" @@ -263,6 +264,101 @@ func TestConvertFromLLMMessage(t *testing.T) { } } +// TestAgentConfigNoFantasyImport verifies AgentConfig can be populated with +// every field — including CoreTools, ExtraTools, and ToolWrapper — using +// only SDK-owned types. This test deliberately does not import +// "charm.land/fantasy"; the package compiling at all is the proof that the +// SDK no longer leaks the dependency name through AgentConfig. +// +// Regression test for https://github.com/mark3labs/kit/issues/30. +func TestAgentConfigNoFantasyImport(t *testing.T) { + myTool := kit.NewTool[struct{}]("noop", "does nothing", func(_ context.Context, _ struct{}) (kit.ToolOutput, error) { + return kit.TextResult("ok"), nil + }) + + wrapperCalled := false + cfg := kit.AgentConfig{ + SystemPrompt: "you are a tester", + MaxSteps: 5, + StreamingEnabled: true, + CoreTools: []kit.Tool{myTool}, + ExtraTools: []kit.Tool{myTool}, + DisableCoreTools: false, + ToolWrapper: func(in []kit.Tool) []kit.Tool { + wrapperCalled = true + return in + }, + OnMCPServerLoaded: func(_ string, _ int, _ error) {}, + } + + if cfg.SystemPrompt != "you are a tester" { + t.Errorf("SystemPrompt = %q, want %q", cfg.SystemPrompt, "you are a tester") + } + if cfg.MaxSteps != 5 { + t.Errorf("MaxSteps = %d, want 5", cfg.MaxSteps) + } + if !cfg.StreamingEnabled { + t.Error("StreamingEnabled = false, want true") + } + if len(cfg.CoreTools) != 1 { + t.Errorf("CoreTools len = %d, want 1", len(cfg.CoreTools)) + } + if len(cfg.ExtraTools) != 1 { + t.Errorf("ExtraTools len = %d, want 1", len(cfg.ExtraTools)) + } + + // Exercise the wrapper to confirm the func type is usable. + out := cfg.ToolWrapper(cfg.CoreTools) + if !wrapperCalled { + t.Error("ToolWrapper was not invoked") + } + if len(out) != 1 { + t.Errorf("wrapped tool list len = %d, want 1", len(out)) + } +} + +// TestAgentConfigToolWrapperSignature documents that AgentConfig.ToolWrapper +// uses kit.Tool (not the underlying provider type) in its signature. +func TestAgentConfigToolWrapperSignature(t *testing.T) { + //nolint:staticcheck // QF1011: explicit type asserts the SDK-side func signature. + var _ func([]kit.Tool) []kit.Tool = func(in []kit.Tool) []kit.Tool { return in } + cfg := kit.AgentConfig{ + ToolWrapper: func(in []kit.Tool) []kit.Tool { return in }, + } + if cfg.ToolWrapper == nil { + t.Fatal("ToolWrapper assignment failed") + } +} + +// TestSpinnerFuncSignature verifies SpinnerFunc has the documented signature +// and can be constructed without importing any provider package. +func TestSpinnerFuncSignature(t *testing.T) { + called := false + var sp kit.SpinnerFunc = func(fn func() error) error { + called = true + return fn() + } + err := sp(func() error { return nil }) + if err != nil { + t.Errorf("SpinnerFunc returned err: %v", err) + } + if !called { + t.Error("SpinnerFunc did not invoke fn") + } +} + +// TestHandlerTypesSignatures verifies the SDK-owned handler function types +// can be assigned from plain function literals using only standard library +// types in their signatures (no provider-package import required). +func TestHandlerTypesSignatures(t *testing.T) { + var _ kit.ToolCallHandler = func(_, _, _ string) {} + var _ kit.ToolExecutionHandler = func(_, _, _ string, _ bool) {} + var _ kit.ToolResultHandler = func(_, _, _, _, _ string, _ bool) {} + var _ kit.ResponseHandler = func(_ string) {} + var _ kit.StreamingResponseHandler = func(_ string) {} + var _ kit.ToolCallContentHandler = func(_ string) {} +} + // containsStr is a tiny helper to avoid importing strings in test. func containsStr(s, substr string) bool { return len(s) >= len(substr) && (s == substr || len(s) > 0 && indexStr(s, substr) >= 0)