Feature Description
Refactor kit.New() to give each Kit instance an isolated, per-instance viper config store (via viper.New()), and add an ergonomic functional-options constructor (NewAgent + With*). This targets the two things with real user impact — config isolation and ergonomics — without purging viper from the SDK's import graph.
This supersedes #38, which proposed removing viper entirely. That framing was re-scoped after mapping viper's transitive reach (~38 call sites across 5 internal packages plus the global ModelsRegistry singleton): full removal is high-risk, high-churn config plumbing whose only payoff is a cosmetic dependency-graph metric. viper does not leak per AGENTS.md's definition (it never appears in an exported symbol, type, godoc, or struct field — only in function bodies, which the SDK rules explicitly permit).
Motivation / Use Case
-
Fix the actual bug — global-state race. viper's process-global singleton means two kit.New() calls in the same process clobber each other's config. viperInitMu only serializes the construction window; it does not isolate long-lived Kit instances. Subagent spawning, tests, and any multi-Kit embedding scenario are all affected. The existing TODO comments in kit.go already flag exactly this fix:
refactor New to use a per-instance *viper.Viper (constructed via viper.New()) so each Kit owns its own isolated config store
-
Ergonomics. New(ctx, *Options) is verbose for simple programmatic setups. A functional-options front door reads better and makes "option not provided" a natural unset signal.
-
Keep CLI/SDK config behavior identical. Both layers continue to resolve model, max-tokens, per-model settings, and .kit.yml through the same viper engine — no parallel config code path to keep in lockstep.
Non-Goals
- Removing the
github.com/spf13/viper dependency from pkg/kit or the internal packages. go list -deps ./pkg/kit | grep viper is expected to remain non-empty.
- Re-implementing viper's precedence /
IsSet() tri-state / AutomaticEnv / env-substitution by hand.
Proposed Implementation
Part A — Per-instance config store (the real fix)
- Construct a fresh store in
New(): v := viper.New() instead of touching the global viper.* package functions.
- Thread that
*viper.Viper explicitly through the construction path:
kitsetup.BuildProviderConfig(v *viper.Viper) and SetupAgent(...) take the instance instead of reading the global.
internal/config loaders (LoadAndValidateConfig, GetField, InitConfig, LoadConfigWithEnvSubstitution) accept a *viper.Viper.
internal/models custom-model / model-settings loading takes the instance (so the per-Kit config feeds the registry rather than the global store).
internal/extensions/runner.go's single config read takes the instance.
- Store the
*viper.Viper on the Kit struct so runtime mutators (SetThinkingLevel, SetModel) read/write the instance, not the global.
- Drop
viperInitMu — no longer needed once state is per-instance.
- Preserve the existing tri-state precedence contract exactly (
IsSet() semantics for max-tokens, sampling params, thinking-level; the sdkDefaultMaxTokens floor; per-model defaults and right-sizing). With a per-instance store this is unchanged — we still call v.IsSet(...).
- Update the "Global viper state warning" godoc on
Options/New to reflect that instances are now isolated.
Boundary note: cmd/ keeps using viper for flag/env/config precedence. The CLI can either share its store with the SDK or hand values in via Options — both stay valid since the SDK store is now independent.
Part B — Ergonomic constructor (purely additive)
Keep New(ctx, *Options) as-is (not deprecated — it remains the escape hatch for MCPConfig, InProcessMCPServers, etc.). Layer functional options on top:
func NewAgent(ctx context.Context, opts ...Option) (*Kit, error) {
o := &Options{}
for _, fn := range opts { fn(o) }
return New(ctx, o)
}
type Option func(*Options)
func WithModel(m string) Option { return func(o *Options) { o.Model = m } }
func WithSystemPrompt(p string) Option { return func(o *Options) { o.SystemPrompt = p } }
func WithStreaming(b bool) Option { return func(o *Options) { o.Streaming = b } }
func WithMaxTokens(n int) Option { return func(o *Options) { o.MaxTokens = n } }
func WithTools(t ...Tool) Option { return func(o *Options) { o.Tools = t } }
func Ephemeral() Option { return func(o *Options) { o.NoSession = true } }
Option is func(*Options) — no dependency leakage. Add godoc on every exported symbol.
Resolve the existing Streaming bool latent edge case here: New() currently does viper.Set("stream", opts.Streaming) unconditionally, so an SDK caller can't leave it "unset" (false always wins). Make a deliberate, documented choice (honor the bool as-is via a WithStreaming default of true, or switch the underlying handling) as part of this work.
Acceptance Criteria
- Two
kit.New() / kit.NewAgent() calls in the same process get independent config — setting model/thinking-level on one does not affect the other. (Add a regression test that fails against the current global-state implementation.)
viperInitMu is removed.
NewAgent + With* / Ephemeral() exist, are documented, and are covered by unit tests.
- Existing tri-state precedence tests (
max-tokens, sampling params, thinking-level) still pass unchanged.
New(ctx, *Options) behavior is unchanged for existing callers (README, examples/sdk/*).
go test ./... -race and golangci-lint run pass.
Out of Scope / Follow-up
If full viper removal is ever revisited, it should be a separate, deliberately-scoped effort with its own design (viper-free internal/config.Settings + env-substituting loader threaded through the internal packages and the global registry). Not part of this issue.
Feature Description
Refactor
kit.New()to give eachKitinstance an isolated, per-instance viper config store (viaviper.New()), and add an ergonomic functional-options constructor (NewAgent+With*). This targets the two things with real user impact — config isolation and ergonomics — without purging viper from the SDK's import graph.This supersedes #38, which proposed removing viper entirely. That framing was re-scoped after mapping viper's transitive reach (~38 call sites across 5 internal packages plus the global
ModelsRegistrysingleton): full removal is high-risk, high-churn config plumbing whose only payoff is a cosmetic dependency-graph metric. viper does not leak perAGENTS.md's definition (it never appears in an exported symbol, type, godoc, or struct field — only in function bodies, which the SDK rules explicitly permit).Motivation / Use Case
Fix the actual bug — global-state race. viper's process-global singleton means two
kit.New()calls in the same process clobber each other's config.viperInitMuonly serializes the construction window; it does not isolate long-lived Kit instances. Subagent spawning, tests, and any multi-Kit embedding scenario are all affected. The existing TODO comments inkit.goalready flag exactly this fix:Ergonomics.
New(ctx, *Options)is verbose for simple programmatic setups. A functional-options front door reads better and makes "option not provided" a natural unset signal.Keep CLI/SDK config behavior identical. Both layers continue to resolve
model,max-tokens, per-model settings, and.kit.ymlthrough the same viper engine — no parallel config code path to keep in lockstep.Non-Goals
github.com/spf13/viperdependency frompkg/kitor the internal packages.go list -deps ./pkg/kit | grep viperis expected to remain non-empty.IsSet()tri-state /AutomaticEnv/ env-substitution by hand.Proposed Implementation
Part A — Per-instance config store (the real fix)
New():v := viper.New()instead of touching the globalviper.*package functions.*viper.Viperexplicitly through the construction path:kitsetup.BuildProviderConfig(v *viper.Viper)andSetupAgent(...)take the instance instead of reading the global.internal/configloaders (LoadAndValidateConfig,GetField,InitConfig,LoadConfigWithEnvSubstitution) accept a*viper.Viper.internal/modelscustom-model / model-settings loading takes the instance (so the per-Kit config feeds the registry rather than the global store).internal/extensions/runner.go's single config read takes the instance.*viper.Viperon theKitstruct so runtime mutators (SetThinkingLevel,SetModel) read/write the instance, not the global.viperInitMu— no longer needed once state is per-instance.IsSet()semantics formax-tokens, sampling params,thinking-level; thesdkDefaultMaxTokensfloor; per-model defaults and right-sizing). With a per-instance store this is unchanged — we still callv.IsSet(...).Options/Newto reflect that instances are now isolated.Part B — Ergonomic constructor (purely additive)
Keep
New(ctx, *Options)as-is (not deprecated — it remains the escape hatch forMCPConfig,InProcessMCPServers, etc.). Layer functional options on top:Optionisfunc(*Options)— no dependency leakage. Add godoc on every exported symbol.Acceptance Criteria
kit.New()/kit.NewAgent()calls in the same process get independent config — setting model/thinking-level on one does not affect the other. (Add a regression test that fails against the current global-state implementation.)viperInitMuis removed.NewAgent+With*/Ephemeral()exist, are documented, and are covered by unit tests.max-tokens, sampling params,thinking-level) still pass unchanged.New(ctx, *Options)behavior is unchanged for existing callers (README,examples/sdk/*).go test ./... -raceandgolangci-lint runpass.Out of Scope / Follow-up
If full viper removal is ever revisited, it should be a separate, deliberately-scoped effort with its own design (viper-free
internal/config.Settings+ env-substituting loader threaded through the internal packages and the global registry). Not part of this issue.