feat(sdk): expose generation and provider params on Options#7
Merged
Conversation
Adds programmatic overrides on kit.Options for the model/provider knobs that were previously only reachable through viper.Set() — letting SDK consumers (web apps, services, embedded agents) configure kit fully in-code without polluting global viper state or shipping .kit.yml. Generation parameters: - MaxTokens int (max output tokens per response) - ThinkingLevel string (off/low/medium/high) - Temperature *float32 - TopP *float32 - TopK *int32 - FrequencyPenalty *float32 - PresencePenalty *float32 Sampling params use pointer types so explicit 0 is distinguishable from unset; nil leaves provider/per-model defaults in place. Provider configuration: - ProviderAPIKey string - ProviderURL string - TLSSkipVerify bool Implementation just pushes Options values into viper inside New(), so all existing downstream code (BuildProviderConfig, SetModel, modelSettings lookups, runtime model switching) picks them up uniformly without any new code paths. Tests added for MaxTokens, ThinkingLevel, and ProviderAPIKey.
Previously setSDKDefaults() registered viper.SetDefault for max-tokens,
temperature, top-p, top-k, frequency/presence-penalty, and thinking-level.
viper.SetDefault makes IsSet() return true, which silently suppressed
per-model defaults (ApplyModelSettings) and automatic right-sizing
(rightSizeMaxTokens) for every SDK-created Kit — and for CLI runs too,
since cmd/root.go routes through kit.New. Effective max-tokens for
claude-sonnet-4-5 was pinned at 4096 instead of 32768.
- Drop SetDefault for all IsSet-sensitive keys; keep only model,
system-prompt, stream, num-gpu-layers, main-gpu.
- Apply a 4096 max-tokens floor directly on the *models.ProviderConfig
struct in kit.New() when nothing else resolved a value. Keeps
viper.IsSet("max-tokens") == false so rightSizeMaxTokens and
per-model maxTokens overrides still fire.
- Update Options.MaxTokens / ThinkingLevel godoc to describe the real
precedence chain.
- Strengthen tests: add Temperature subtest; add
TestNewPreservesIsSetSemantics regression covering all seven keys;
split TestNewWithProviderOptions into three subtests including
Options-beats-viper-state and ProviderURL propagation; add
resetViper helper so subtests don't bleed state.
- Document the new SDK fields (MaxTokens, ThinkingLevel, Temperature,
TopP, TopK, FrequencyPenalty, PresencePenalty, ProviderAPIKey,
ProviderURL, TLSSkipVerify) in README, skills/kit-sdk, and the www
configuration / sdk/options / sdk/overview pages, including a
dedicated precedence table.
|
Connected to Huly®: KIT-7 |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 38 minutes and 21 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe kit SDK now supports programmatic configuration of generation parameters (MaxTokens, ThinkingLevel, sampling controls) and provider settings (API key, URL, TLS) directly via the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 2
🧹 Nitpick comments (3)
pkg/kit/kit_test.go (1)
230-257:_ = errswallows creation failure silently.The subtest deliberately tolerates
kit.Newfailing (model validation, provider handshake) and only asserts the viper state. That's a reasonable scope, but_ = errreads as a lint suppression without intent. Consider a shortt.Logfso a future reader (or CI flake investigation) can tell whether the override assertion is being exercised pre- or post-failure:Suggested tweak
- if host != nil { - defer func() { _ = host.Close() }() - } - _ = err + if host != nil { + defer func() { _ = host.Close() }() + } + if err != nil { + t.Logf("kit.New returned error (tolerated): %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/kit/kit_test.go` around lines 230 - 257, Replace the silent suppression of the kit.New error with a test log so failures are visible during CI debugging: instead of `_ = err` call t.Logf (e.g., t.Logf("kit.New error: %v", err)) after creating host so the test still tolerates creation failure but records whether kit.New failed, leaving the existing host cleanup and the viper.ProviderAPIKey assertion intact (look for kit.New, kit.Options.ProviderAPIKey, and viper.GetString in this subtest).pkg/kit/kit.go (1)
1245-1255: Minor: theopts.MaxTokens == 0conjunct is redundant.If
opts.MaxTokens > 0the earlierviper.Set("max-tokens", opts.MaxTokens)at line 1126 guaranteesproviderConfig.MaxTokens != 0afterBuildProviderConfig. SoproviderConfig.MaxTokens == 0alone is sufficient. Not a bug — harmless defensive coding — flagging only for readability.Optional simplification
- if providerConfig.MaxTokens == 0 && opts.MaxTokens == 0 { + if providerConfig.MaxTokens == 0 { providerConfig.MaxTokens = sdkDefaultMaxTokens }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/kit/kit.go` around lines 1245 - 1255, The condition in BuildProviderConfig (the block that applies sdkDefaultMaxTokens) uses a redundant conjunct opts.MaxTokens == 0; simplify the guard to check only providerConfig.MaxTokens == 0 and then set providerConfig.MaxTokens = sdkDefaultMaxTokens when true, keeping sdkDefaultMaxTokens as the fallback; this removes the unnecessary dependency on opts.MaxTokens while preserving the current behavior.README.md (1)
549-589: LGTM — SDK example and precedence notes are accurate.Matches the
kit.Optionsgodoc and the test-verified behavior (TestNewWithGenerationOptions,TestNewWithProviderOptions). Worth noting theptrhelper isn't shown — consider a 2-linefunc ptr[T any](v T) *T { return &v }snippet nearby so copy-pasters don't hit a compile error, thoughskills/kit-sdk/SKILL.mdalready definesptrFloat32.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 549 - 589, Add a short generic ptr helper so the README example compiles (the example uses ptr for Temperature) — implement a small two-line helper named ptr (returns a pointer to a value) and place it adjacent to the code sample, or alternatively add a short note pointing readers to the existing ptrFloat32 helper in skills/kit-sdk/SKILL.md; keep references to kit.Options and the tests (TestNewWithGenerationOptions, TestNewWithProviderOptions) intact so readers can correlate behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/kit/kit_test.go`:
- Around line 150-195: The test TestNewPreservesIsSetSemantics contains an
unnecessary KIT_* env-var skip guard and use of upper() because SkipConfig: true
prevents InitConfig() from registering viper env handling; remove the
os.Getenv(envVar) check and the upper() dependency from the test
(TestNewPreservesIsSetSemantics) so the test only asserts viper.IsSet for
SDK-set keys, and fix InitConfig() to call
viper.SetEnvKeyReplacer(strings.NewReplacer("-", "_")) in addition to the
existing viper.SetEnvPrefix("KIT") and viper.AutomaticEnv() so hyphenated keys
like "max-tokens" map to KIT_MAX_TOKENS when config init is enabled.
In `@pkg/kit/kit.go`:
- Around line 1119-1158: The code mutates viper's global state inside kit.New
via viper.Set (e.g., when handling Options fields like MaxTokens, Temperature,
ProviderAPIKey), causing values to leak between Kit instances; add a clear godoc
comment on the Options type and kit.New explaining that Options are applied by
mutating the global viper store (via viper.Set) and therefore Kit is not
isolated across multiple New() calls, referencing kit.New, Options, viper.Set,
viperInitMu, resetViper, and downstream readers like SetModel/GetThinkingLevel;
include a short migration note advising consumers to call resetViper() between
constructions or to avoid creating multiple Kits in the same process, and add a
TODO suggesting a future refactor to use a per-call viper.New() instance to
fully isolate config.
---
Nitpick comments:
In `@pkg/kit/kit_test.go`:
- Around line 230-257: Replace the silent suppression of the kit.New error with
a test log so failures are visible during CI debugging: instead of `_ = err`
call t.Logf (e.g., t.Logf("kit.New error: %v", err)) after creating host so the
test still tolerates creation failure but records whether kit.New failed,
leaving the existing host cleanup and the viper.ProviderAPIKey assertion intact
(look for kit.New, kit.Options.ProviderAPIKey, and viper.GetString in this
subtest).
In `@pkg/kit/kit.go`:
- Around line 1245-1255: The condition in BuildProviderConfig (the block that
applies sdkDefaultMaxTokens) uses a redundant conjunct opts.MaxTokens == 0;
simplify the guard to check only providerConfig.MaxTokens == 0 and then set
providerConfig.MaxTokens = sdkDefaultMaxTokens when true, keeping
sdkDefaultMaxTokens as the fallback; this removes the unnecessary dependency on
opts.MaxTokens while preserving the current behavior.
In `@README.md`:
- Around line 549-589: Add a short generic ptr helper so the README example
compiles (the example uses ptr for Temperature) — implement a small two-line
helper named ptr (returns a pointer to a value) and place it adjacent to the
code sample, or alternatively add a short note pointing readers to the existing
ptrFloat32 helper in skills/kit-sdk/SKILL.md; keep references to kit.Options and
the tests (TestNewWithGenerationOptions, TestNewWithProviderOptions) intact so
readers can correlate behavior.
🪄 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: f9e96f2e-e58d-4a40-b2c2-ed93b222773e
📒 Files selected for processing (8)
README.mdpkg/kit/config.gopkg/kit/kit.gopkg/kit/kit_test.goskills/kit-sdk/SKILL.mdwww/pages/configuration.mdwww/pages/sdk/options.mdwww/pages/sdk/overview.md
The SDK last-resort MaxTokens floor is applied in kit.New() when
Options.MaxTokens, KIT_MAX_TOKENS, .kit.yml, and per-model defaults
are all unset. It was 4096 (inherited from the old setSDKDefaults
viper default) while the CLI --max-tokens cobra default is 8192.
Bump the floor to 8192 so SDK and CLI callers start from the same
base value before rightSizeMaxTokens runs, then update README,
skills/kit-sdk/SKILL.md, and www/pages/{configuration,sdk/options}.md
to match.
- InitConfig now installs a viper env key replacer so keys like "max-tokens" bind to KIT_MAX_TOKENS under AutomaticEnv; previously hyphenated keys silently missed their documented env overrides. - Simplify TestNewPreservesIsSetSemantics: with SkipConfig: true no env bindings are registered, so the os.Getenv guard and upper() helper were dead weight. Remove both and drop the unused helper.
The SDK applies Options by calling viper.Set on viper's process-global store, which means two Kits constructed in the same process are not isolated from each other: the second New overwrites the first's keys, and downstream readers (SetModel, GetThinkingLevel, BuildProviderConfig) observe the most recent value. - Add a 'Global viper state warning' block to the Options godoc explaining the leak, the zero-value-does-not-clear gotcha, and pointing at viper.Reset() as the migration workaround. - Add a matching warning to the New godoc so consumers discover the constraint from either entry point. - Detach the viperInitMu godoc (previously lodged inside New's comment block) and clarify that the mutex only guards the construction window, not instance isolation. - Add a TODO noting the proper fix: refactor to a per-call viper.New() instance so each Kit owns its own config store.
TestDetectMediaType/.go fails on CI images (Ubuntu mime-support) where /etc/mime.types registers '.go → text/x-go', because mime.TypeByExtension reads those files at init. The test intended to exercise the 'unknown extension falls through to text/plain' branch but used a real extension, making the assertion environment-dependent. Replace '.go' with '.kitsyntheticext', an invented extension that no system MIME database registers. The fallback path is now exercised deterministically on any host.
10 tasks
This was referenced Jun 18, 2026
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.
Let SDK consumers configure Kit fully in-code without reaching into
viper's global state or shipping a
.kit.yml.Why
Before this PR, downstream apps that embed Kit had to do this:
Now:
What's in
New
OptionsfieldsGeneration parameters
MaxTokensint0= auto-resolve; non-zero suppresses auto right-sizingThinkingLevelstring"off","low","medium","high"Temperature*float320.0≠ unsetTopP*float32TopK*int32FrequencyPenalty*float32PresencePenalty*float32Provider configuration
ProviderAPIKeystringProviderURLstringTLSSkipVerifybooltruePrecedence (highest → lowest)
Options.XKIT_Xenv var.kit.ymlmodelSettings/customModels.params)MaxTokens = 4096)Sampling params that remain
nilat step 6 are left out of the provider callentirely, so the LLM library applies its own default.
Correctness fix (2nd commit)
The initial implementation regressed
viper.IsSet()semantics: theoriginal
setSDKDefaults()calledviper.SetDefaultfor every param,which makes
IsSet()return true. That silently suppressed per-modeldefaults (
ApplyModelSettings) and auto right-sizing (rightSizeMaxTokens)for every SDK-created Kit — and for every CLI run, since
cmd/root.goalso routes through
kit.New. EffectiveMaxTokensforclaude-sonnet-4-5 was pinned at 4096 instead of 32768.
Fix:
SetDefaultfor all IsSet-sensitive keys (max-tokens,temperature,top-p,top-k,frequency-penalty,presence-penalty,thinking-level). Keep onlymodel,system-prompt,stream,num-gpu-layers,main-gpu.MaxTokensfloor directly on the*ProviderConfigstruct in
kit.New()when nothing else resolved a value. Keepsviper.IsSet("max-tokens") == falseso right-sizing and per-modelmaxTokensstill fire.Empirically verified: SDK
Kit.MaxTokens()for claude-sonnet-4-5before fix =
4096, after fix =32768.Tests
TestNewWithGenerationOptions/{MaxTokens,ThinkingLevel,Temperature}—assert each Options field propagates end-to-end.
TestNewPreservesIsSetSemantics(regression) — asserts that when nogeneration Options field is set,
viper.IsSet()returnsfalsefor allseven keys. Uses
SkipConfig: trueto isolate from~/.kit.yml.TestNewWithProviderOptions/{succeeds,Options beats viper,ProviderURL}—including a subtest that writes a placeholder key to viper first, then
confirms
Options.ProviderAPIKeywins.resetViper()helper so subtests don't bleed state.Backwards compatibility
Optionsfields unchanged.go test -race ./...clean.go vet ./...clean.Docs
Added or updated in the same commit:
README.md— expanded Options code block, fixed--max-tokensdefault(was
4096, actually8192with auto right-sizing).skills/kit-sdk/SKILL.md— new fields + cheat-sheet table.www/pages/sdk/options.md— restructured into grouped sections, addedPrecedence section.
www/pages/sdk/overview.md— new "Generation & provider overrides" section.www/pages/configuration.md— precedence summary pointing at SDK options.Suggested release: minor bump (
v0.55.0) — purely additive API.Test plan
Summary by CodeRabbit
Release Notes
New Features
MaxTokens,ThinkingLevel,Temperature,TopP,TopK,FrequencyPenalty,PresencePenalty)ProviderAPIKey,ProviderURL,TLSSkipVerify)--max-tokensto 8192 with auto-scaling up to 32768Documentation