refactor(sdk): drop unreachable kit.AgentConfig surface#67
Merged
Conversation
Today the SDK exposes a DebugLogger interface (pkg/kit/types.go) but no public path to install one — the only consumer of the field is the unexported kit.AgentConfig.toInternal() method, which itself is not reachable from outside the package. As a result, embedders that want to forward Kit's low-level engine + MCP tool plumbing debug output into their own logging system (slog, zap, charm/log, an in-app TUI panel, etc.) have no option but the on/off Debug bool, which always installs the built-in SimpleDebugLogger / BufferedDebugLogger. This change closes that gap on the supported Options / functional-option construction path: - pkg/kit/kit.go: add Options.DebugLogger DebugLogger. When non-nil it is used directly and the Debug bool is ignored; the supplied logger's IsDebugEnabled() controls whether downstream code emits messages. - pkg/kit/options.go: add WithDebugLogger(l DebugLogger) Option. - internal/kitsetup/setup.go: add AgentSetupOptions.DebugLogger and switch SetupAgent's logger selection so the caller-supplied logger wins unconditionally; otherwise the existing Debug + UseBufferedLogger branch picks the built-in implementation. No behaviour change when DebugLogger is nil. - pkg/kit/kit.go: wire opts.DebugLogger into setupOpts so the New() path threads it through. - pkg/kit/viper_isolation_test.go: add TestWithDebugLoggerPlumbing and TestWithDebugLoggerNilClears covering the option-to-field contract and later-options-override semantics consistent with the other With* helpers. - pkg/kit/README.md: list WithDebugLogger in the helper inventory. Notes: - kit.DebugLogger and tools.DebugLogger are structurally identical (LogDebug(string) / IsDebugEnabled() bool), so the SDK value flows into the internal field without a conversion. - This is purely additive on the SDK surface and does not touch kit.AgentConfig — that field already carried a DebugLogger, but the AgentConfig path is unreachable from outside the package today.
|
Connected to Huly®: KIT-68 |
kit.AgentConfig (pkg/kit/types.go) and its toInternal converter were exposed as the documented "low-level / advanced consumer" path for agent construction, but the converter was unexported and not wired into any public constructor — neither New(*Options) nor NewAgent(...Option) accept an AgentConfig. The only call sites were the dedicated agent_config_internal_test.go (same-package internal test) and two fantasy-import regression tests in types_test.go. Net effect today: no SDK consumer outside pkg/kit can populate or use kit.AgentConfig in any way. The type, the converter, the dedicated test file, and a chain of godoc cross-references all exist purely for their own sake — they don't enlarge what consumers can do, but they do enlarge the SDK's stability contract (every field becomes a public shape the internal agent layer can't refactor freely). The companion PR added Options.DebugLogger + WithDebugLogger so the last functional capability AgentConfig was documented to enable — installing a custom debug logger — is reachable through the supported construction path. With that wired, AgentConfig has no remaining purpose. Changes: - pkg/kit/types.go: remove the AgentConfig struct and its toInternal() method. Drop the now-unused internal/agent and internal/tools imports. Update the DebugLogger godoc to point at Options.DebugLogger and WithDebugLogger instead of AgentConfig. - pkg/kit/agent_config_internal_test.go: deleted (208 LOC). It exercised the unexported toInternal() method directly; with the method gone the test has no subject. - pkg/kit/types_test.go: rename TestAgentConfigNoFantasyImport to TestOptionsNoFantasyImport and rewrite it against Options (SystemPrompt, MaxSteps, Streaming, Tools, ExtraTools, DisableCoreTools, OnMCPServerLoaded). The original test also asserted ToolWrapper field semantics; that capability migrates to the hook system (OnBeforeToolCall / OnAfterToolResult), already covered by hooks_test.go, so the assertion is dropped with a pointer in the godoc. TestAgentConfigToolWrapperSignature replaced by TestToolSliceSignature, which still pins that []kit.Tool is the user-visible slice type for every tool-related SDK surface — the no-fantasy-import contract the original test guarded. - pkg/kit/mcp_tasks.go: update the MCPTaskConfig godoc to stop referencing AgentConfig. MCPTaskConfig stays — it is still emitted through Options.MCPTask* fields and used as the engine-facing config type. - pkg/kit/README.md: drop the kit.AgentConfig line from the type inventory. internal/agent.AgentConfig is untouched and remains the internal construction shape. With the public type gone the internal one can evolve freely without breaking the SDK contract. Verification: - go build ./pkg/... ./internal/... ./cmd/... — clean - go vet ./pkg/... ./internal/... ./cmd/... — clean - go test -race -timeout 300s ./... — all packages pass
ezynda3
force-pushed
the
refactor/drop-agentconfig
branch
from
2efc28d to
276c787
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds ChangesWithDebugLogger option end-to-end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
- README.md: add WithDebugLogger to the functional-options helper list - pkg/kit/README.md: expand the Debug row and add a DebugLogger row in the Options field summary - www/pages/sdk/overview.md: add WithDebugLogger to the helpers table with a note that it overrides WithDebug when set - www/pages/sdk/options.md: surface DebugLogger in the example, expand the Debug field description, add a DebugLogger row to the Core fields table, and add a "Custom debug logger" section with the interface signature and a log/slog adapter example
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
1 participant
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.
Summary
Removes the public
kit.AgentConfigtype and its unreachable conversion helper, replacing the one missing capability it carried (custom debug logger) with a properOptionsfield +WithDebugLoggerfunctional option. After this PRinternal/agent.AgentConfigis purely internal and can be refactored without breaking the SDK contract.Supersedes and closes #66.
Background
kit.AgentConfig(pkg/kit/types.go) was documented as the "low-level / advanced consumer" path for agent construction. Its conversion helper,toInternal(), was unexported. Neitherkit.New(*Options)norkit.NewAgent(...Option)accepts anAgentConfig, so no SDK consumer outsidepkg/kitcould populate or use it in any way.The only call sites were:
pkg/kit/agent_config_internal_test.go— the dedicated same-package test exercisingtoInternal()directly.pkg/kit/types_test.go— two regression tests verifying the struct can be populated without importingcharm.land/fantasy.Both rely entirely on same-package access. From the outside the type, the converter, the godoc cross-references in
mcp_tasks.go/DebugLogger, and the README entry have no consumer-visible function — but they do enlarge the SDK's stability contract: everyAgentConfigfield is a public shape the internal agent layer can't refactor freely.Changes (commit 1 — feat:
WithDebugLogger)The companion commit adds
Options.DebugLogger+WithDebugLoggerso the last functional capabilityAgentConfigwas documented to enable — installing a custom debug logger — is reachable through the supported construction path.pkg/kit/kit.go: newOptions.DebugLogger DebugLoggerfield. When non-nil it is used directly and theDebugbool is ignored; the supplied logger'sIsDebugEnabled()controls whether downstream code emits messages.pkg/kit/options.go: newWithDebugLogger(l DebugLogger) Option.internal/kitsetup/setup.go: newAgentSetupOptions.DebugLogger;SetupAgent's logger selection switches on it so a caller-supplied logger wins unconditionally; otherwise the existingDebug+UseBufferedLoggerbranch picks the built-in implementation. No behaviour change when nil.pkg/kit/viper_isolation_test.go:TestWithDebugLoggerPlumbing+TestWithDebugLoggerNilClears.pkg/kit/README.md: helper inventory updated.Changes (commit 2 — refactor: remove
AgentConfig)With debug-logger wiring in place, the public type is removed:
pkg/kit/types.go: drop theAgentConfigstruct andtoInternal()method. Drop the now-unusedinternal/agentandinternal/toolsimports.DebugLogger's godoc rewritten to point atOptions.DebugLogger/WithDebugLogger.pkg/kit/agent_config_internal_test.go: deleted (208 LOC). It exercisedtoInternal()directly; with the method gone the test has no subject.pkg/kit/types_test.go:TestAgentConfigNoFantasyImport→TestOptionsNoFantasyImport, rewritten againstOptions(SystemPrompt,MaxSteps,Streaming,Tools,ExtraTools,DisableCoreTools,OnMCPServerLoaded).TestAgentConfigToolWrapperSignature→TestToolSliceSignature. The original assertedToolWrapperfield semantics; that capability migrates to the hook system (OnBeforeToolCall/OnAfterToolResult), already covered byhooks_test.go, so the assertion is dropped with a pointer in the godoc. The new test still pins that[]kit.Toolis the user-visible slice type for every tool-related SDK surface — the no-fantasy-import contract the original guarded.pkg/kit/mcp_tasks.go:MCPTaskConfiggodoc updated to stop referencingAgentConfig. The type itself stays — it is still emitted throughOptions.MCPTask*fields and used as the engine-facing config type.pkg/kit/README.md:kit.AgentConfigremoved from the type inventory.internal/agent.AgentConfigis untouched and remains the internal construction shape. With the public type gone the internal one can evolve freely without breaking the SDK contract.Net surface impact
kit.AgentConfig(struct + 14 fields) — no external consumer existed.Options.DebugLogger,kit.WithDebugLogger.TestAgentConfigNoFantasyImport→TestOptionsNoFantasyImport;TestAgentConfigToolWrapperSignature→TestToolSliceSignature.WithDebugLoggercommit.Migration
There is no real-world migration because no consumer can currently reach
AgentConfigfrom outsidepkg/kit. Hypothetical reachers (e.g. forks that vendored their own constructor) map cleanly ontoOptions:AgentConfig)Options)ModelConfigModel+ProviderAPIKey/ProviderURL/ sampling paramsMCPConfigMCPConfigSystemPromptSystemPrompt(orWithSystemPrompt)MaxStepsMaxStepsStreamingEnabledStreaming *bool(orWithStreaming)AuthHandlerMCPAuthHandlerTokenStoreFactoryMCPTokenStoreFactoryCoreToolsTools(orWithTools)DisableCoreToolsDisableCoreToolsExtraToolsExtraTools(orWithExtraTools)ToolWrapperKit.OnBeforeToolCall/Kit.OnAfterToolResult(hook system)OnMCPServerLoadedOnMCPServerLoadedDebugLoggerDebugLogger(orWithDebugLogger) — added in this PRMCPTaskConfigMCPTaskMode/MCPTaskTimeout/MCPTaskTTL/MCPTaskPollInterval/MCPTaskMaxPollInterval/MCPTaskProgressVerification
go build ./pkg/... ./internal/... ./cmd/...— cleango vet ./pkg/... ./internal/... ./cmd/...— cleango test -race -timeout 300s ./...— all packages passNotes
kit.DebugLoggerandtools.DebugLoggerare structurally identical (LogDebug(string)/IsDebugEnabled() bool), so the SDK value flows into the internal field without a conversion.kit.Toolalias tofantasy.AgentToolis unchanged. It complies with theAGENTS.mdno-dependency-name-leakage rule (the alias iskit.Tool; the dependency name appears nowhere in identifiers or godoc).Summary by CodeRabbit
Release Notes
New Features
WithDebugLoggeroption to supply custom debug loggers, allowing users to route engine and MCP debug output into their own logging infrastructure. Custom loggers take precedence over the built-inDebugflag.Documentation
DebugLoggeroption and interface, including usage examples and implementation guidance for custom loggers.