feat(kit): isolate viper config per Kit instance + add NewAgent#42
Merged
Conversation
- Give each kit.New()/NewAgent() call an isolated *viper.Viper store so multiple Kit instances in one process no longer clobber each other's config; runtime mutators (SetModel, SetThinkingLevel) touch only the owning instance, making subagent spawning and multi-Kit embedding race-free - Thread the per-instance store through internal/config, internal/models (ProviderConfig.ConfigStore), internal/kitsetup, and the extension runner, with a nil -> process-global fallback so the CLI is unaffected - Share the global store when Options.CLI != nil to preserve cobra flag bindings (also opted in for internal/acpserver) - Remove viperInitMu; preserve the tri-state IsSet precedence contract and sdkDefaultMaxTokens floor - Add ergonomic NewAgent + functional options (WithModel, WithStreaming, Ephemeral, etc.); NewAgent defaults streaming on, opt out via WithStreaming(false). New(ctx, *Options) behavior is unchanged - Add config-isolation regression test and NewAgent/option coverage; document NewAgent and per-instance isolation in README Fixes #40
- Add "Functional options (NewAgent)" and "Per-instance config isolation" sections to the docs site SDK overview, with an options table and a "when to use which" constructor comparison - Cross-reference NewAgent from the SDK options page and correct the now per-instance ProviderAPIKey precedence wording - Document NewAgent + With* helpers and config isolation in pkg/kit/README and list NewAgent/Option in the API reference - Show the NewAgent constructor in the SDK examples getting-started snippet
|
Connected to Huly®: KIT-43 |
📝 Walkthrough🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/kit/kit_test.go (1)
225-256:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdjust the test to cover the intended global-vs-instance precedence (or rename it).
In the SDK path (
opts.CLI == nil),pkg/kit/kit.go:Newuses a freshviper.New()instance store, and then directlyv.Set("provider-api-key", opts.ProviderAPIKey). That means theviper.Set("provider-api-key", ...)on the process-global store inpkg/kit/kit_test.gocan’t influencehost.ConfigStringForTest(...), so the subtest name/comment (“beats pre-existing viper state”) no longer matches what’s actually being exercised.Seed the instance store’s
provider-api-key(via env var/config file that the instance loads), or remove the globalviper.Setand rename the subtest to reflect onlyOptions.ProviderAPIKeypropagation.🤖 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/kit_test.go` around lines 225 - 256, The test name/comments claim to verify global-vs-instance precedence but New (pkg/kit/kit.go:New) creates a fresh viper store and writes opts.ProviderAPIKey directly, so the process-global viper.Set in the test does not affect host.ConfigStringForTest; update the test to actually seed the instance store (e.g., provide the provider-api-key via the instance's env/config loading path so the Kit's internal viper sees a pre-existing value) or else remove the global viper.Set call and rename the subtest to reflect it's only asserting that Options.ProviderAPIKey is propagated to the instance (referencing kit.New and host.ConfigStringForTest to locate where the behavior is asserted).pkg/kit/kit.go (1)
1911-1927:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSubagents no longer inherit the parent's provider/runtime config.
After this PR, each child
New(...)starts from a freshviper.New()store, butchildOptsonly carries a small subset of the parent state. Parent overrides likeProviderAPIKey,ProviderURL,TLSSkipVerify,MaxTokens, sampling params, and runtimeSetThinkingLevelchanges are dropped, so the child can run with different provider settings than the parent despite theSubagentcontract saying they are shared.🤖 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/kit.go` around lines 1911 - 1927, child subagents are losing parent provider/runtime config because childOpts (passed to New in New(ctx, childOpts)) only contains a subset of m.opts; extend propagation to include provider and runtime settings by copying fields like ProviderAPIKey, ProviderURL, TLSSkipVerify, MaxTokens, sampling parameters, and any runtime state set via SetThinkingLevel from m.opts into childOpts before calling New; update or add a helper (similar to inheritMCPTaskOptions) to merge these provider/runtime fields so children created by New inherit the parent's provider overrides and runtime configuration.
🧹 Nitpick comments (4)
pkg/kit/viper_isolation_test.go (1)
24-24: ⚡ Quick win
WithStreaming(false)assertion is tautological.
ostarts as&kit.Options{}, soo.Streamingis already thefalsezero value. The check at Lines 53-55 would pass even ifWithStreamingwere a no-op, so it doesn't actually verify the setter. Seed the opposite value first (or assert both directions) to make it meaningful.♻️ Suggested change
- kit.WithStreaming(false), + kit.WithStreaming(true),- if o.Streaming { - t.Error("WithStreaming(false): expected Streaming=false") + if !o.Streaming { + t.Error("WithStreaming(true): expected Streaming=true") }Note: the default-on / opt-out behavior of
NewAgentis already covered byTestNewAgentDefaultsStreamingOnandTestNewAgentStreamingOptOut, so this case is free to verify the plumbing direction that the bare-struct default can't.Also applies to: 53-55
🤖 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/viper_isolation_test.go` at line 24, The test is asserting WithStreaming(false) against the zero-value Options which makes the check tautological; update the test so it actually verifies the setter by first seeding a non-default value (e.g., set o.Streaming = true) before applying kit.WithStreaming(false) or add a complementary assertion that Applying kit.WithStreaming(true) flips it to true; locate the test using the Options struct and the WithStreaming option helper in pkg/kit/viper_isolation_test.go and change the setup/assertion so the option actually changes the pre-seeded value rather than relying on the zero-value.internal/models/providers.go (1)
27-29: ⚡ Quick winReorder the imports into stdlib → third-party → local groups.
viperwas added after the local import block, so this import section now violates the repository's Go ordering rule.Suggested diff
"charm.land/fantasy/providers/openrouter" "charm.land/fantasy/providers/vercel" openaisdk "github.com/charmbracelet/openai-go" + "github.com/spf13/viper" "github.com/mark3labs/kit/internal/auth" - "github.com/spf13/viper" )As per coding guidelines, "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 `@internal/models/providers.go` around lines 27 - 29, Reorder the import block so imports are grouped as stdlib → third-party → local with blank lines between groups; specifically move "github.com/spf13/viper" into the third-party group (above the local "github.com/mark3labs/kit/internal/auth") and ensure any standard library imports remain in the first group; update the import block in internal/models/providers.go accordingly.internal/kitsetup/setup.go (1)
12-17: ⚡ Quick winReorder the imports into stdlib → third-party → local groups.
The new
viperimport is sitting in the local block, so this file no longer matches the repository's Go import ordering rule.Suggested diff
import ( "context" "fmt" "charm.land/fantasy" + "github.com/spf13/viper" "github.com/mark3labs/kit/internal/agent" "github.com/mark3labs/kit/internal/config" "github.com/mark3labs/kit/internal/extensions" "github.com/mark3labs/kit/internal/models" "github.com/mark3labs/kit/internal/tools" - "github.com/spf13/viper" )As per coding guidelines, "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 `@internal/kitsetup/setup.go` around lines 12 - 17, The imports in setup.go are not grouped correctly: move the third-party import "github.com/spf13/viper" out of the local block and place it in the third-party group (after stdlib imports and before local imports like "github.com/mark3labs/..."), ensure blank lines between stdlib, third-party, and local groups, and then run goimports/gofmt to apply the canonical ordering; look for the import block that contains "github.com/spf13/viper" to make this change.internal/models/custom.go (1)
21-33: ⚡ Quick winUse the project logger for these new config-parse warnings.
These new warning paths still emit through the stdlib
logpackage, so they bypass the structured logging backend the rest of the Go code is supposed to use.Suggested diff
import ( "log" "os" "strings" + charmLog "github.com/charmbracelet/log" "github.com/spf13/viper" ) @@ var customModels map[string]CustomModelConfig if err := v.UnmarshalKey("customModels", &customModels); err != nil { - log.Printf("Warning: Failed to parse customModels: %v", err) + charmLog.Warn("failed to parse customModels", "error", err) return nil } @@ var settings map[string]GenerationParamsConfig if err := v.UnmarshalKey("modelSettings", &settings); err != nil { - log.Printf("Warning: Failed to parse modelSettings: %v", err) + charmLog.Warn("failed to parse modelSettings", "error", err) return nil }As per coding guidelines, "Use github.com/charmbracelet/log for structured logging".
Also applies to: 83-95
🤖 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 `@internal/models/custom.go` around lines 21 - 33, The warnings in loadCustomModelsFrom (and the similar block around lines 83-95) use the stdlib log.Printf; replace those with the project's structured logger (the github.com/charmbracelet/log logger used across the codebase) so config-parse errors go through the structured backend — update the imports to use the project logger and change the log.Printf calls (e.g., the "Warning: Failed to parse customModels: %v" message emitted when v.UnmarshalKey fails and the other warning paths) to call the structured logger (preserving the message and error variable) via the package-level logger used elsewhere.
🤖 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.
Inline comments:
In `@internal/acpserver/session.go`:
- Around line 45-48: The code currently sets CLI: &kit.CLIOptions{} when
creating ACP sessions which causes kit.New (see kit.New and Options.CLI) to
reuse the global viper, leaking mutable config across sessionRegistry entries;
remove the CLI pointer so each session gets its own isolated config (do not pass
CLI: &kit.CLIOptions{}) and instead, if CLI defaults are required, capture a
snapshot of the global CLI config and explicitly apply that snapshot to each new
*kit.Kit instance (e.g., populate session config via a copy/seed function before
calling kit.New) to avoid shared global viper usage and cross-instance bleed
(addresses SetModel/SetThinkingLevel races).
In `@internal/config/merger.go`:
- Around line 33-42: The current error returns use "%v" which loses the error
chain; update the two returns after v.Unmarshal and config.Validate to wrap the
underlying error with fmt.Errorf("...: %w", err) so callers can use errors.Is /
errors.As; locate the unmarshalling error return in the function that calls
v.Unmarshal (and the validation error return after config.Validate) and replace
their fmt.Errorf formats to use "%w", and ensure fixEnvironmentCase(v, config)
remains between unmarshalling and validation.
In `@pkg/kit/config.go`:
- Around line 91-97: The early return in initConfig causes env handling
(SetEnvPrefix, SetEnvKeyReplacer, AutomaticEnv) to be skipped when a configFile
is provided; update initConfig so the environment overrides are always
configured before loading a file: ensure calls that set up env handling (e.g.,
v.SetEnvPrefix("KIT"), v.SetEnvKeyReplacer(...), v.AutomaticEnv()) happen
unconditionally and then call loadConfigWithEnvSubstitution(v, configFile) when
configFile != "", rather than returning before the env setup; keep the same
function name initConfig and existing call to loadConfigWithEnvSubstitution but
reorder/adjust flow so explicit config files do not disable KIT_* overrides.
In `@pkg/kit/kit.go`:
- Around line 1247-1250: The code unconditionally calls v.Set("stream",
opts.Streaming) which forces stream=false when a zero-valued Options is passed;
change this to only set the viper key when the caller explicitly provided a
streaming option (e.g. introduce a sentinel like Options.StreamingSet bool or
make Streaming a *bool) and then wrap the v.Set call in a conditional (if
opts.StreamingSet { v.Set("stream", opts.Streaming) } or if opts.Streaming !=
nil { v.Set("stream", *opts.Streaming) }) so New(ctx, &Options{}) will not
override config/env/default streaming.
- Around line 1221-1230: The config initialization guard mistakenly relies on
v.GetString("model") which setSDKDefaults(v) always populates for SDK callers,
causing initConfig to be skipped and .kit*/KIT_* MCP config not to load; update
the condition to not depend on "model" — call initConfig(v, opts.ConfigFile,
false) whenever opts.SkipConfig is false (i.e., remove the v.GetString("model")
check) so that initConfig runs for SDK usage and subsequent BuildProviderConfig
/ LoadAndValidateConfigFrom see the loaded config.
---
Outside diff comments:
In `@pkg/kit/kit_test.go`:
- Around line 225-256: The test name/comments claim to verify global-vs-instance
precedence but New (pkg/kit/kit.go:New) creates a fresh viper store and writes
opts.ProviderAPIKey directly, so the process-global viper.Set in the test does
not affect host.ConfigStringForTest; update the test to actually seed the
instance store (e.g., provide the provider-api-key via the instance's env/config
loading path so the Kit's internal viper sees a pre-existing value) or else
remove the global viper.Set call and rename the subtest to reflect it's only
asserting that Options.ProviderAPIKey is propagated to the instance (referencing
kit.New and host.ConfigStringForTest to locate where the behavior is asserted).
In `@pkg/kit/kit.go`:
- Around line 1911-1927: child subagents are losing parent provider/runtime
config because childOpts (passed to New in New(ctx, childOpts)) only contains a
subset of m.opts; extend propagation to include provider and runtime settings by
copying fields like ProviderAPIKey, ProviderURL, TLSSkipVerify, MaxTokens,
sampling parameters, and any runtime state set via SetThinkingLevel from m.opts
into childOpts before calling New; update or add a helper (similar to
inheritMCPTaskOptions) to merge these provider/runtime fields so children
created by New inherit the parent's provider overrides and runtime
configuration.
---
Nitpick comments:
In `@internal/kitsetup/setup.go`:
- Around line 12-17: The imports in setup.go are not grouped correctly: move the
third-party import "github.com/spf13/viper" out of the local block and place it
in the third-party group (after stdlib imports and before local imports like
"github.com/mark3labs/..."), ensure blank lines between stdlib, third-party, and
local groups, and then run goimports/gofmt to apply the canonical ordering; look
for the import block that contains "github.com/spf13/viper" to make this change.
In `@internal/models/custom.go`:
- Around line 21-33: The warnings in loadCustomModelsFrom (and the similar block
around lines 83-95) use the stdlib log.Printf; replace those with the project's
structured logger (the github.com/charmbracelet/log logger used across the
codebase) so config-parse errors go through the structured backend — update the
imports to use the project logger and change the log.Printf calls (e.g., the
"Warning: Failed to parse customModels: %v" message emitted when v.UnmarshalKey
fails and the other warning paths) to call the structured logger (preserving the
message and error variable) via the package-level logger used elsewhere.
In `@internal/models/providers.go`:
- Around line 27-29: Reorder the import block so imports are grouped as stdlib →
third-party → local with blank lines between groups; specifically move
"github.com/spf13/viper" into the third-party group (above the local
"github.com/mark3labs/kit/internal/auth") and ensure any standard library
imports remain in the first group; update the import block in
internal/models/providers.go accordingly.
In `@pkg/kit/viper_isolation_test.go`:
- Line 24: The test is asserting WithStreaming(false) against the zero-value
Options which makes the check tautological; update the test so it actually
verifies the setter by first seeding a non-default value (e.g., set o.Streaming
= true) before applying kit.WithStreaming(false) or add a complementary
assertion that Applying kit.WithStreaming(true) flips it to true; locate the
test using the Options struct and the WithStreaming option helper in
pkg/kit/viper_isolation_test.go and change the setup/assertion so the option
actually changes the pre-seeded value rather than relying on the zero-value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a5e7f6a-f277-4612-a853-a7ca96ded6b8
📒 Files selected for processing (17)
README.mdexamples/sdk/README.mdinternal/acpserver/session.gointernal/config/merger.gointernal/extensions/runner.gointernal/kitsetup/setup.gointernal/models/custom.gointernal/models/providers.gopkg/kit/README.mdpkg/kit/config.gopkg/kit/export_test.gopkg/kit/kit.gopkg/kit/kit_test.gopkg/kit/options.gopkg/kit/viper_isolation_test.gowww/pages/sdk/options.mdwww/pages/sdk/overview.md
- Isolate each ACP session's config store instead of sharing the global
viper, preventing per-session SetModel/SetThinkingLevel races; seed the
root-command flag values (model, thinking-level, provider URL/key) so
`kit acp -m <model>` is still honored
- Run initConfig for isolated SDK stores by gating on opts.CLI instead of
v.GetString("model"), which setSDKDefaults always populates and thus
skipped .kit.yml / KIT_* loading for SDK callers
- Configure KIT_* env overrides unconditionally in initConfig so passing an
explicit config file no longer disables environment variable support
- Wrap config unmarshal/validate errors with %w to preserve the error chain
- Change Options.Streaming from bool to *bool so a zero-valued Options no longer forces stream=false; New only sets the key when non-nil, letting streaming resolve through the precedence chain (env -> config -> default true). This also fixes the CLI path, which never set the field - Mirror the existing sampling-parameter pointer pattern instead of adding a separate StreamingSet sentinel, keeping Options internally consistent - Update WithStreaming/NewAgent, subagent, and ACP callers to the pointer form; add regression tests for the nil-default and explicit opt-out paths - Update SDK docs (README, pkg/kit/README, options page) with the ptrBool helper and *bool semantics
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@pkg/kit/kit.go`:
- Around line 1925-1933: childOpts is created with only a few fields, so
instance-only provider/runtime settings stored on m.v (e.g., ProviderAPIKey,
ProviderURL, TLSSkipVerify, ThinkingLevel, MaxTokens, sampler overrides, etc.)
are not propagated to New(...), causing Subagent() to lose effective config;
before calling New(...) populate childOpts (or clone the instance store) by
reading the current values from m.v and copying them into the corresponding
Options fields (or by cloning m.v into the child's config store), ensuring
Options (and any runtime setters like SetThinkingLevel) are reflected in the
child; update the code around childOpts construction to snapshot all relevant
keys from m.v into childOpts or supply a cloned viper instance so Subagent()
inherits the parent provider/runtime config.
In `@pkg/kit/viper_isolation_test.go`:
- Around line 192-205: The test depends on the KIT_STREAM env var; update the
test around the kit.New call to ensure KIT_STREAM cannot influence behavior by
saving and clearing/restoring the env or using t.Setenv: before creating the Kit
instance, capture the current value of "KIT_STREAM" (or use
t.Setenv("KIT_STREAM", "")), unset or set it to a controlled value so
Options.Streaming==nil is evaluated in isolation, then restore the original
value after the test; reference symbols: kit.New, kit.Options{SkipConfig: true},
and k.ConfigBoolForTest("stream") to locate where to add the env
save/clear/restore.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 766f9d93-312d-44c9-9354-6e9b5b3b75c2
📒 Files selected for processing (7)
README.mdinternal/acpserver/session.gopkg/kit/README.mdpkg/kit/kit.gopkg/kit/options.gopkg/kit/viper_isolation_test.gowww/pages/sdk/options.md
✅ Files skipped from review due to trivial changes (1)
- pkg/kit/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- internal/acpserver/session.go
- pkg/kit/options.go
- Copy the parent's effective provider/runtime config (API key, URL, TLS, thinking level, max-tokens, samplers) onto child Options in Kit.Subagent. After the per-instance viper isolation, the child's isolated store only re-loaded .kit.yml / KIT_*, silently dropping config the parent set via programmatic Options or runtime setters like SetThinkingLevel - Preserve the IsSet tri-state for max-tokens and samplers so per-model defaults still apply on the child when the parent left them unset - Add TestInheritProviderConfig covering propagation, unset keys, and nil-safety
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/kit/mcp_tasks_test.go (1)
169-246: ⚡ Quick winConsider adding partial tri-state coverage.
The test thoroughly verifies the all-set and none-set scenarios. Adding a subtest for partial tri-state coverage would strengthen confidence that each sampler's
IsSetcheck works independently. For example, set onlytemperatureand verify thatTemperatureis populated whileTopP,TopK,FrequencyPenalty, andPresencePenaltyremain nil.🧪 Example partial coverage subtest
+ t.Run("partial sampler coverage", func(t *testing.T) { + // Only set temperature and top-k; leave top-p and penalties unset. + v := viper.New() + v.Set("temperature", 0.8) + v.Set("top-k", 50) + + child := &Options{} + inheritProviderConfig(child, v) + + if child.Temperature == nil || *child.Temperature != 0.8 { + t.Errorf("Temperature = %v, want 0.8", child.Temperature) + } + if child.TopK == nil || *child.TopK != 50 { + t.Errorf("TopK = %v, want 50", child.TopK) + } + if child.TopP != nil || child.FrequencyPenalty != nil || child.PresencePenalty != nil { + t.Error("unset samplers must remain nil") + } + })🤖 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_test.go` around lines 169 - 246, Add a subtest inside TestInheritProviderConfig that exercises a partial tri-state scenario: create a viper store setting only "temperature" (e.g. 0.25) and no other sampler keys, call inheritProviderConfig(child, v) with child := &Options{}, then assert that child.Temperature is non-nil and equals the set value while child.TopP, child.TopK, child.FrequencyPenalty, and child.PresencePenalty remain nil and MaxTokens stays zero and ThinkingLevel is unchanged; keep the existing nil/empty checks and ensure inheritProviderConfig and Options are used to locate the behavior being tested.
🤖 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_test.go`:
- Around line 169-246: Add a subtest inside TestInheritProviderConfig that
exercises a partial tri-state scenario: create a viper store setting only
"temperature" (e.g. 0.25) and no other sampler keys, call
inheritProviderConfig(child, v) with child := &Options{}, then assert that
child.Temperature is non-nil and equals the set value while child.TopP,
child.TopK, child.FrequencyPenalty, and child.PresencePenalty remain nil and
MaxTokens stays zero and ThinkingLevel is unchanged; keep the existing nil/empty
checks and ensure inheritProviderConfig and Options are used to locate the
behavior being tested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b9c034e-26a7-4a03-8221-e7d48c2fb460
📒 Files selected for processing (2)
pkg/kit/kit.gopkg/kit/mcp_tasks_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/kit/kit.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
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.
Description
kit.New()previously wrote all configuration into viper's process-globalstore. Two
kit.New()calls in the same process therefore clobbered eachother's config, and
viperInitMuonly serialized the construction window — itdid not isolate long-lived instances. This affected subagent spawning, tests,
and any multi-Kit embedding scenario.
This PR gives each
Kitits own isolated*viper.Viper(viaviper.New()),threaded explicitly through the construction path and stored on the
Kitstruct so runtime mutators (
SetModel,SetThinkingLevel) read/write theinstance — not the global singleton. The internal packages (
config,models,kitsetup,extensions) gained instance-aware variants with anil → process-globalfallback, so the CLI (which binds cobra flags to theglobal viper) is unaffected: when
Options.CLI != nilthe Kit shares theglobal store.
viperInitMuis removed. The existing tri-stateIsSet()precedence contract and the
sdkDefaultMaxTokensfloor are preserved exactly.It also adds an ergonomic, purely-additive functional-options constructor.
New(ctx, *Options)is unchanged.NewAgentdefaults streaming on (passWithStreaming(false)to opt out),resolving the latent edge case where
Options.Streaming'sfalsezero valuealways won.
Fixes #40
Type of Change
How Has This Been Tested?
TestKitConfigIsolation— a regression test that fails against theold global-state implementation (setting the thinking level on one Kit must
not affect another in the same process).
NewAgent/With*/Ephemeralunit coverage, including thestreaming-default and opt-out paths.
max-tokens, samplingparams,
thinking-level) to assert on the per-instance store via test-onlyaccessors; their semantics are unchanged and still pass.
go test -race ./...andgolangci-lint runpass.www/) builds cleanly (tome build).Checklist
Additional Information
Acceptance criteria (issue #40): ✅ independent config per instance
(regression test) · ✅
viperInitMuremoved · ✅NewAgent+With*/Ephemeraldocumented & tested · ✅ tri-state precedence preserved · ✅New(ctx, *Options)unchanged for existing callers · ✅ viper dependencyintentionally retained (non-goal honored).
Backward compatibility: fully backward compatible.
New(ctx, *Options)keeps its signature and behavior; the CLI/ACP server opt into the shared
global store via
Options.CLI, preserving cobra flag bindings.Notable files
Core fix:
pkg/kit/kit.go— per-instance*viper.Viperon theKitstruct;New,SetModel,SetThinkingLevel,GetThinkingLevel,ExecuteCompletion,ReloadExtensionsuse it;viperInitMuremovedpkg/kit/config.go— instance-awareinitConfig/setSDKDefaults/loadConfigWithEnvSubstitution(global wrappers retained)internal/config/merger.go—LoadAndValidateConfigFrom(*viper.Viper)internal/models/{custom,providers}.go—ProviderConfig.ConfigStore,LoadModelSettingsFrom, instance-awareApplyModelSettings/isExplicitlySetinternal/kitsetup/setup.go—BuildProviderConfig(v)+AgentSetupOptions.Viperinternal/extensions/runner.go—Runner.SetConfigStoreinternal/acpserver/session.go— opts into the shared global storeNew API:
pkg/kit/options.go—NewAgent,Option, andWith*/EphemeralhelpersTests:
pkg/kit/viper_isolation_test.go,pkg/kit/export_test.go,pkg/kit/kit_test.goDocs:
README.md,pkg/kit/README.md,examples/sdk/README.md,www/pages/sdk/overview.md,www/pages/sdk/options.mdJudgment calls worth a look: the model-registry singleton's custom-model
catalog stays process-global, while per-instance
modelSettings/precedence isthreaded via
ProviderConfig.ConfigStore; the CLI shares the global store;NewAgentdefaults streaming on.Summary by CodeRabbit