From 1fb94179390c66e98ca85cf5f5bfd7331a088aab Mon Sep 17 00:00:00 2001 From: Ed Zynda Date: Tue, 2 Jun 2026 14:12:02 +0300 Subject: [PATCH] fix(kit): make Options.Streaming a *bool to honor unset - 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 --- README.md | 2 +- internal/acpserver/session.go | 3 +- pkg/kit/README.md | 2 +- pkg/kit/kit.go | 33 ++++++++++++------- pkg/kit/options.go | 10 +++--- pkg/kit/viper_isolation_test.go | 58 +++++++++++++++++++++++++++++++-- www/pages/sdk/options.md | 7 ++-- 7 files changed, 92 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index f47c2107..f426e73d 100644 --- a/README.md +++ b/README.md @@ -556,7 +556,7 @@ host, err := kit.New(ctx, &kit.Options{ SystemPrompt: "You are a helpful bot", ConfigFile: "/path/to/config.yml", MaxSteps: 10, - Streaming: true, + Streaming: ptr(true), // *bool: nil = unset (default true), &false = off Quiet: true, // Generation parameters (override env/config/per-model defaults) diff --git a/internal/acpserver/session.go b/internal/acpserver/session.go index 2b45111a..937cc6ad 100644 --- a/internal/acpserver/session.go +++ b/internal/acpserver/session.go @@ -45,10 +45,11 @@ func (r *sessionRegistry) create(ctx context.Context, cwd string) (*acpSession, // the process-global store (which cobra populated from flags) so launching // `kit acp -m [--thinking-level ...] [--provider-url ...]` is still // honored; .kit.yml and KIT_* env vars are loaded per session by kit.New. + streamOn := true kitInstance, err := kit.New(ctx, &kit.Options{ SessionDir: cwd, Quiet: true, - Streaming: true, + Streaming: &streamOn, Model: viper.GetString("model"), ThinkingLevel: viper.GetString("thinking-level"), ProviderURL: viper.GetString("provider-url"), diff --git a/pkg/kit/README.md b/pkg/kit/README.md index fc4fefbd..fcaafb4a 100644 --- a/pkg/kit/README.md +++ b/pkg/kit/README.md @@ -89,7 +89,7 @@ host, err := kit.New(ctx, &kit.Options{ SystemPrompt: "You are a helpful bot", // Override system prompt ConfigFile: "/path/to/config.yml", // Use specific config file MaxSteps: 10, // Override max steps - Streaming: true, // Enable streaming + Streaming: ptrBool(true), // *bool: nil = unset (default true), &false = off Quiet: true, // Suppress debug output // Session options diff --git a/pkg/kit/kit.go b/pkg/kit/kit.go index 06fb4839..81db4942 100644 --- a/pkg/kit/kit.go +++ b/pkg/kit/kit.go @@ -890,10 +890,17 @@ type Options struct { SystemPrompt string // Override system prompt ConfigFile string // Override config file path MaxSteps int // Override max steps (0 = use default) - Streaming bool // Enable streaming (default from config) - Quiet bool // Suppress debug output - Tools []Tool // Custom tool set. If empty, AllTools() is used. - ExtraTools []Tool // Additional tools added alongside core/MCP/extension tools. + + // Streaming enables or disables streaming output. It is a pointer so the + // SDK can distinguish "unset" (nil) from an explicit choice, mirroring the + // sampling-parameter fields below. nil leaves streaming to the precedence + // chain (env → .kit.yml → default true); a non-nil value forces it. Prefer + // [WithStreaming] for the functional-options API. + Streaming *bool + + Quiet bool // Suppress debug output + Tools []Tool // Custom tool set. If empty, AllTools() is used. + ExtraTools []Tool // Additional tools added alongside core/MCP/extension tools. // Generation parameters. These override the corresponding values from // .kit.yml / KIT_* environment variables. Leaving a field at its @@ -1250,7 +1257,12 @@ func New(ctx context.Context, opts *Options) (*Kit, error) { if opts.MaxSteps > 0 { v.Set("max-steps", opts.MaxSteps) } - v.Set("stream", opts.Streaming) + // Only override streaming when the caller explicitly set it. Otherwise + // leave the precedence chain (env → config → default true) untouched so a + // zero-valued Options does not silently force stream=false. + if opts.Streaming != nil { + v.Set("stream", *opts.Streaming) + } // Generation parameter overrides. Each Options field, when set, // is pushed into the instance store here so the existing downstream @@ -1905,19 +1917,18 @@ func (m *Kit) Subagent(ctx context.Context, cfg SubagentConfig) (*SubagentResult // Create child Kit instance. Pass the parent's loaded MCP config to // avoid re-loading and re-validating config for the child. - // Streaming must be explicitly enabled — Options.Streaming defaults to - // false, and New() writes Set("stream", opts.Streaming) on the child's - // isolated store. Without this, the subagent would potentially hit + // Streaming is enabled explicitly — without it, non-streaming can hit // provider-level differences (e.g. Anthropic non-streaming timeouts with - // extended thinking). The child gets its own config store, so this no - // longer affects any other concurrent caller. + // extended thinking). The child gets its own config store, so this does not + // affect any other concurrent caller. + streamOn := true childOpts := &Options{ Model: model, SystemPrompt: systemPrompt, Tools: tools, NoSession: cfg.NoSession, Quiet: true, - Streaming: true, + Streaming: &streamOn, MCPConfig: m.mcpConfig, } // Propagate the parent's MCP task configuration so a child subagent diff --git a/pkg/kit/options.go b/pkg/kit/options.go index ec6f1e5a..d40c711a 100644 --- a/pkg/kit/options.go +++ b/pkg/kit/options.go @@ -27,9 +27,9 @@ type Option func(*Options) // ) func NewAgent(ctx context.Context, opts ...Option) (*Kit, error) { // Streaming defaults to true for the ergonomic constructor — this is the - // natural expectation for interactive agents and avoids the latent - // "false always wins" edge case of the raw Options.Streaming zero value. - o := &Options{Streaming: true} + // natural expectation for interactive agents. WithStreaming(false) overrides it. + streamOn := true + o := &Options{Streaming: &streamOn} for _, fn := range opts { fn(o) } @@ -46,7 +46,9 @@ func WithSystemPrompt(p string) Option { return func(o *Options) { o.SystemPromp // WithStreaming enables or disables streaming responses. [NewAgent] enables // streaming by default, so pass WithStreaming(false) to opt out. -func WithStreaming(b bool) Option { return func(o *Options) { o.Streaming = b } } +func WithStreaming(b bool) Option { + return func(o *Options) { o.Streaming = &b } +} // WithMaxTokens sets the maximum output tokens per LLM response. A value of 0 // lets the precedence chain (env → config → per-model → SDK floor) resolve a diff --git a/pkg/kit/viper_isolation_test.go b/pkg/kit/viper_isolation_test.go index 9655e65f..3cca7063 100644 --- a/pkg/kit/viper_isolation_test.go +++ b/pkg/kit/viper_isolation_test.go @@ -50,8 +50,10 @@ func TestOptionFunctionsPlumbing(t *testing.T) { if o.ConfigFile != "/tmp/.kit.yml" { t.Errorf("WithConfigFile: got %q", o.ConfigFile) } - if o.Streaming { - t.Error("WithStreaming(false): expected Streaming=false") + if o.Streaming == nil { + t.Error("WithStreaming: expected Streaming to be set (non-nil)") + } else if *o.Streaming { + t.Error("WithStreaming(false): expected *Streaming=false") } if !o.Debug { t.Error("WithDebug: expected Debug=true") @@ -176,3 +178,55 @@ func TestNewAgentStreamingOptOut(t *testing.T) { t.Error("WithStreaming(false) should disable streaming") } } + +// TestNewZeroOptionsKeepsStreamingDefault is the regression test for the +// unconditional `v.Set("stream", opts.Streaming)` bug: a zero-valued Options +// (Streaming == nil) must NOT force stream=false. With Streaming unset, +// streaming resolves through the precedence chain, whose SDK default is true. +func TestNewZeroOptionsKeepsStreamingDefault(t *testing.T) { + if os.Getenv("ANTHROPIC_API_KEY") == "" { + t.Skip("Skipping test: ANTHROPIC_API_KEY not set") + } + + ctx := context.Background() + k, err := kit.New(ctx, &kit.Options{ + Model: "anthropic/claude-sonnet-4-5-20250929", + Quiet: true, + NoSession: true, + SkipConfig: true, // isolate from any ~/.kit.yml / env stream setting + }) + if err != nil { + t.Fatalf("New failed: %v", err) + } + defer func() { _ = k.Close() }() + + if !k.ConfigBoolForTest("stream") { + t.Error("zero-valued Options must not force stream=false; expected the default (true)") + } +} + +// TestNewStreamingExplicitOptOut verifies that a raw Options can still disable +// streaming by setting Streaming to a pointer to false. +func TestNewStreamingExplicitOptOut(t *testing.T) { + if os.Getenv("ANTHROPIC_API_KEY") == "" { + t.Skip("Skipping test: ANTHROPIC_API_KEY not set") + } + + streamOff := false + ctx := context.Background() + k, err := kit.New(ctx, &kit.Options{ + Model: "anthropic/claude-sonnet-4-5-20250929", + Quiet: true, + NoSession: true, + SkipConfig: true, + Streaming: &streamOff, + }) + if err != nil { + t.Fatalf("New failed: %v", err) + } + defer func() { _ = k.Close() }() + + if k.ConfigBoolForTest("stream") { + t.Error("Streaming=&false should disable streaming") + } +} diff --git a/www/pages/sdk/options.md b/www/pages/sdk/options.md index d8f388c3..424c1ce4 100644 --- a/www/pages/sdk/options.md +++ b/www/pages/sdk/options.md @@ -28,7 +28,7 @@ host, err := kit.New(ctx, &kit.Options{ // Behavior MaxSteps: 10, - Streaming: true, + Streaming: ptrBool(true), // *bool: nil = unset (default true), &false = off Quiet: true, Debug: true, @@ -101,7 +101,7 @@ host, err := kit.New(ctx, &kit.Options{ | `SystemPrompt` | `string` | — | System prompt text or file path | | `ConfigFile` | `string` | `~/.kit.yml` | Path to config file | | `MaxSteps` | `int` | `0` | Max agent steps (0 = unlimited) | -| `Streaming` | `bool` | `true` | Enable streaming output | +| `Streaming` | `*bool` | `nil` | Enable streaming output. `nil` leaves it to the precedence chain (env → config → default `true`); `&true`/`&false` forces it. Pointer so unset is distinct from explicit `false`. | | `Quiet` | `bool` | `false` | Suppress output | | `Debug` | `bool` | `false` | Enable debug logging | @@ -124,9 +124,10 @@ defaults for samplers). | `FrequencyPenalty` | `*float32` | — | OpenAI-family frequency penalty. `nil` leaves provider default. | | `PresencePenalty` | `*float32` | — | OpenAI-family presence penalty. `nil` leaves provider default. | -Pointer-typed samplers are populated via a tiny helper: +Pointer-typed fields (`Streaming` and the samplers) are populated via tiny helpers: ```go +func ptrBool(v bool) *bool { return &v } func ptrFloat32(v float32) *float32 { return &v } ```