fix(sdk): stop leaking fantasy types through pkg/kit.AgentConfig (#30)#32
Merged
Conversation
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
|
Connected to Huly®: KIT-33 |
📝 WalkthroughWalkthroughThis PR refactors the ChangesSDK Type System Refactoring
Sequence Diagram(s)sequenceDiagram
participant SDKConsumer as SDK Consumer
participant AgentConfig as AgentConfig (SDK)
participant toInternal as toInternal()
participant InternalAgent as agent.AgentConfig
SDKConsumer->>AgentConfig: populate DebugLogger, AuthHandler, TokenStoreFactory
SDKConsumer->>AgentConfig: set CoreTools, ToolWrapper
SDKConsumer->>AgentConfig: set MCPTaskConfig with Progress callback
AgentConfig->>toInternal: convert SDK config
toInternal->>InternalAgent: wire converted auth/token/debug hooks
toInternal->>InternalAgent: assign tools and tool wrapper
toInternal->>InternalAgent: convert MCPTaskConfig to internal.tools.MCPTaskConfig
InternalAgent-->>SDKConsumer: ready for Kit.New()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the public pkg/kit SDK surface so AgentConfig and related handler types no longer re-export internal/agent aliases that expose charm.land/fantasy.* in consumer code and rendered godoc, aligning with the dependency-agnostic boundary described in AGENTS.md / issue #30.
Changes:
- Replace
kit.AgentConfigand agent handler type aliases with Kit-owned types and add an internal conversion helper(*AgentConfig).toInternal(). - Adjust
pkg/kit.Newto castopts.CLI.SpinnerFuncinto the internal agent spinner function type. - Add regression tests ensuring consumers can use
AgentConfigand handler types without importingcharm.land/fantasy, plus conversion-focused tests fortoInternal(), and update SDK documentation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/kit/types.go | Introduces Kit-owned AgentConfig + handler types, adds toInternal() conversion, and makes SpinnerFunc a named SDK type. |
| pkg/kit/kit.go | Casts SDK SpinnerFunc to internal agent.SpinnerFunc at the setup boundary. |
| pkg/kit/agent_config_internal_test.go | Adds unit tests validating AgentConfig.toInternal() conversion behavior. |
| pkg/kit/types_test.go | Adds compile-time/usage tests proving AgentConfig and handler types don’t require a fantasy import. |
| pkg/kit/README.md | Updates documentation to reflect Kit-owned agent configuration and callback types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+146
to
+157
| 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, | ||
| } |
The first revision of the SDK-owned AgentConfig dropped two fields that internal/agent.AgentConfig carried: DebugLogger (tools.DebugLogger) and MCPTaskConfig (tools.MCPTaskConfig). Restore them with SDK-owned equivalents and wire them through toInternal(). - Add kit.DebugLogger interface (LogDebug / IsDebugEnabled) mirroring tools.DebugLogger. Interface-to-interface assignment is automatic because the method sets match. - Add kit.MCPTaskConfig struct mirroring tools.MCPTaskConfig with SDK types (MCPTaskMode, MCPTaskProgressHandler) and a toToolsConfig() helper that converts at the SDK boundary. - Wire both new fields in (*AgentConfig).toInternal(). - Extend agent_config_internal_test.go with cases for both fields. - Document the additions in pkg/kit/README.md.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/kit/types.go (1)
3-17: ⚡ Quick winReorder imports to match repository grouping rules.
Line 14-16 keeps local imports mixed ahead of third-party imports. Group as stdlib → third-party → local with blank lines.
♻️ Proposed fix
import ( "context" "charm.land/fantasy" + "github.com/mark3labs/mcp-go/client/transport" + "github.com/mark3labs/mcp-go/server" "github.com/mark3labs/kit/internal/agent" "github.com/mark3labs/kit/internal/compaction" "github.com/mark3labs/kit/internal/config" "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" )As per coding guidelines,
**/*.go: "Organize imports in order: stdlib → third-party → local, with blank lines between groups".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/kit/types.go` around lines 3 - 17, The import block in types.go mixes local packages with third-party packages; reorder the imports into three groups separated by blank lines: standard library (e.g., context), third-party (e.g., "charm.land/fantasy", "github.com/mark3labs/mcp-go/client/transport", "github.com/mark3labs/mcp-go/server"), and then local/internal packages (e.g., "github.com/mark3labs/kit/internal/agent", "compaction", "config", "message", "models", "session", "tools"), so move the local github.com/mark3labs/kit/internal/* imports to the last group and ensure blank lines between groups to satisfy the repo import ordering rule.pkg/kit/mcp_tasks.go (1)
136-163: ⚡ Quick winDe-duplicate
toToolsConfigmapping to prevent drift.Line 136-163 duplicates the same field/mode/progress mapping already present in
mcpTaskOptions.toToolsConfig. Extract a shared private helper so future field additions can’t diverge across paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/kit/mcp_tasks.go` around lines 136 - 163, Extract the duplicated mapping logic into a single unexported helper (e.g. mapMCPConfigToTools(cfg MCPTaskConfig) tools.MCPTaskConfig) and have both MCPTaskConfig.toToolsConfig and mcpTaskOptions.toToolsConfig call it; move the shared assignments (DefaultTTL, PollInterval, MaxPollInterval, Timeout, PerServerMode conversion, and Progress wrapper) into that helper and return the populated tools.MCPTaskConfig so each original method can apply any additional method-specific fields afterward.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/kit/mcp_tasks.go`:
- Around line 136-163: Extract the duplicated mapping logic into a single
unexported helper (e.g. mapMCPConfigToTools(cfg MCPTaskConfig)
tools.MCPTaskConfig) and have both MCPTaskConfig.toToolsConfig and
mcpTaskOptions.toToolsConfig call it; move the shared assignments (DefaultTTL,
PollInterval, MaxPollInterval, Timeout, PerServerMode conversion, and Progress
wrapper) into that helper and return the populated tools.MCPTaskConfig so each
original method can apply any additional method-specific fields afterward.
In `@pkg/kit/types.go`:
- Around line 3-17: The import block in types.go mixes local packages with
third-party packages; reorder the imports into three groups separated by blank
lines: standard library (e.g., context), third-party (e.g.,
"charm.land/fantasy", "github.com/mark3labs/mcp-go/client/transport",
"github.com/mark3labs/mcp-go/server"), and then local/internal packages (e.g.,
"github.com/mark3labs/kit/internal/agent", "compaction", "config", "message",
"models", "session", "tools"), so move the local
github.com/mark3labs/kit/internal/* imports to the last group and ensure blank
lines between groups to satisfy the repo import ordering rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59860ead-4487-4186-a3c7-1d72105f0b74
📒 Files selected for processing (6)
pkg/kit/README.mdpkg/kit/agent_config_internal_test.gopkg/kit/kit.gopkg/kit/mcp_tasks.gopkg/kit/types.gopkg/kit/types_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
pkg/kit/is documented as the dependency-agnostic public SDK surface, butAgentConfigand the agent handler types weretype X = internal/agent.Xaliases over structs and signatures that referencedcharm.land/fantasy.*. Any external consumer that wanted to populateCoreTools,ExtraTools, orToolWrapperwas forced toimport "charm.land/fantasy", andfantasy.*showed up in the rendered godoc onpkg.go.dev.This PR makes
AgentConfigand the handler types Kit-owned, so the SDK contract is genuinely dependency-agnostic — matching the rules inAGENTS.md. An unexported(*AgentConfig).toInternal()translates tointernal/agent.AgentConfigat the SDK boundary, wherefantasyreferences are allowed. Becausekit.Toolis still a type alias for the underlying tool type, slice and function fields convert without allocation.Before (forced
fantasyimport):After (SDK-only):
go doc github.com/mark3labs/kit/pkg/kit AgentConfigno longer mentionsfantasy.*anywhere.Fixes #30
Type of Change
Checklist
go test ./... -race)go vet ./...andgolangci-lint runboth passpkg/kit/README.md"Re-exported Types")Additional Information
Files changed
pkg/kit/types.go—AgentConfigis now a real struct with SDK-typed fields;ToolCallHandler,ToolExecutionHandler,ToolResultHandler,ResponseHandler,StreamingResponseHandler,ToolCallContentHandler, andSpinnerFuncare now SDK-owned named types. Added(*AgentConfig).toInternal()conversion helper.pkg/kit/kit.go— single call-site cast (agent.SpinnerFunc(opts.CLI.SpinnerFunc)) needed becauseSpinnerFuncis no longer an alias.pkg/kit/agent_config_internal_test.go(new) — table-driven tests fortoInternal(): nil receiver, scalar fields, tool slices,ToolWrapperinvocation,OnMCPServerLoaded, and auth/token-factory wiring.pkg/kit/types_test.go— addedTestAgentConfigNoFantasyImport,TestAgentConfigToolWrapperSignature,TestSpinnerFuncSignature,TestHandlerTypesSignatures. Thekit_testpackage file deliberately does not importcharm.land/fantasy; the file compiling is the regression proof for the leak.pkg/kit/README.md— updated the "Re-exported Types" section to record thatAgentConfigand the handler types are now Kit-owned (not aliases).Backwards compatibility
Source-level backwards compatible. All field names, types, and function signatures on
AgentConfigand the handler types are unchanged from a consumer's point of view — the only difference is that the underlying type identities are now SDK-owned. Consumers that previously importedcharm.land/fantasyonly to satisfy these signatures can now drop that import.Verification
go test ./... -race— all packages passgo vet ./...— cleangolangci-lint run ./...— 0 issuesgo doc ./pkg/kit AgentConfig— nofantasy.*referencesscripts/acp_smoke_test.py) againstopencode/kimi-k2.5: 67 streaming updates,stopReason: end_turnSummary by CodeRabbit