From 3c4f0baea9b715b7990603ccb062a6677a2f8f0e Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Wed, 12 Aug 2026 17:12:35 -0700 Subject: [PATCH 1/3] Clear Bedrock compat when tearing down Claude Code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bedrock compat is persisted so a later plain "thv llm setup" keeps it, but it applies to no client other than Claude Code. Tearing Claude Code down therefore left the setting with no consumer, and only an explicit --bedrock-compat=false could clear it. A subsequent setup — possibly against a gateway that no longer forwards to Bedrock — silently re-pinned the Bedrock model IDs and CLAUDE_CODE_DISABLE_EXPERIMENTAL_BETAS the user had just removed, which reads as setup forcing Bedrock unconditionally. Clear the persisted Bedrock config when Claude Code is among the reverted tools. Stickiness across ordinary re-runs is unchanged. --- pkg/llm/config.go | 2 ++ pkg/llm/setup.go | 11 +++++++ pkg/llm/setup_test.go | 68 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+) diff --git a/pkg/llm/config.go b/pkg/llm/config.go index 18b789466c..82a7c4ca60 100644 --- a/pkg/llm/config.go +++ b/pkg/llm/config.go @@ -43,6 +43,8 @@ type Config struct { // BedrockConfig holds settings for configuring Claude Code to reach an LLM // gateway that forwards to AWS Bedrock. It is persisted so that a later plain // "thv llm setup" re-applies these settings rather than silently clearing them. +// Because it only ever applies to Claude Code, tearing Claude Code down clears +// it (see Teardown) so it cannot outlive its only consumer. type BedrockConfig struct { // Compat, when true, writes CLAUDE_CODE_DISABLE_EXPERIMENTAL_BETAS=1 and the // per-tier Bedrock model IDs into Claude Code's settings.json. Bedrock rejects diff --git a/pkg/llm/setup.go b/pkg/llm/setup.go index d4516e29b3..d547da4d06 100644 --- a/pkg/llm/setup.go +++ b/pkg/llm/setup.go @@ -191,6 +191,9 @@ func Setup( // configured tools. An error is returned when targetTool is non-empty but not // found in the configured tool list. // +// Reverting Claude Code also clears the persisted Bedrock compat settings, which +// apply to no other client, so a later "thv llm setup" does not re-apply them. +// // If secretsProvider is non-nil and purgeTokens is true, cached OIDC tokens // are deleted after the config update succeeds. func Teardown( @@ -241,6 +244,14 @@ func Teardown( // nothing on disk has changed and the caller can retry. if err := provider.UpdateLLMConfig(func(c *Config) error { c.ConfiguredTools = remaining + // Bedrock compat only ever applies to Claude Code, so tearing Claude Code + // down leaves it with no consumer. Clear it rather than letting it persist: + // otherwise a later "thv llm setup" — possibly against a gateway that no + // longer forwards to Bedrock — silently re-pins the Bedrock model IDs the + // user just removed, and only an explicit --bedrock-compat=false clears it. + if isTarget(toRevert, claudeCodeClient) { + c.Bedrock = BedrockConfig{} + } if purgeTokens { c.OIDC.CachedRefreshTokenRef = "" c.OIDC.CachedTokenExpiry = time.Time{} diff --git a/pkg/llm/setup_test.go b/pkg/llm/setup_test.go index aa0ae2c841..7a716718b1 100644 --- a/pkg/llm/setup_test.go +++ b/pkg/llm/setup_test.go @@ -355,6 +355,74 @@ func TestTeardown_PurgeTokens_ClearsConfigRefsAndDeletesSecrets(t *testing.T) { assert.Equal(t, []string{"cursor"}, gm.reverted) } +// TestTeardown_BedrockClearedWithClaudeCode verifies that the persisted Bedrock +// settings are cleared exactly when Claude Code is reverted — they apply to no +// other client, so leaving them would let a later "thv llm setup" silently +// re-pin the Bedrock model IDs the user just tore down. +func TestTeardown_BedrockClearedWithClaudeCode(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + configured []ToolConfig + targetTool string + wantCompat bool + wantRemains []string + }{ + { + name: "reverting claude-code clears bedrock", + configured: []ToolConfig{{Tool: "claude-code", ConfigPath: "/tmp/claude.json"}}, + targetTool: "claude-code", + wantCompat: false, + }, + { + name: "reverting another tool keeps bedrock for claude-code", + configured: []ToolConfig{ + {Tool: "claude-code", ConfigPath: "/tmp/claude.json"}, + {Tool: "cursor", ConfigPath: "/tmp/cursor.json"}, + }, + targetTool: "cursor", + wantCompat: true, + wantRemains: []string{"claude-code"}, + }, + { + name: "reverting all tools clears bedrock", + configured: []ToolConfig{ + {Tool: "claude-code", ConfigPath: "/tmp/claude.json"}, + {Tool: "cursor", ConfigPath: "/tmp/cursor.json"}, + }, + targetTool: "", + wantCompat: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + provider := &stubConfigUpdater{cfg: Config{ + ConfiguredTools: tt.configured, + Bedrock: BedrockConfig{Compat: true, Enable1M: true}, + }} + + var stdout, stderr bytes.Buffer + err := Teardown(context.Background(), &stdout, &stderr, + &stubGatewayManager{}, tt.targetTool, false, provider, nil) + require.NoError(t, err) + + assert.Equal(t, tt.wantCompat, provider.cfg.Bedrock.Compat) + // Enable1M rides the same config and must not outlive Compat. + assert.Equal(t, tt.wantCompat, provider.cfg.Bedrock.Enable1M) + + var remaining []string + for _, tc := range provider.cfg.ConfiguredTools { + remaining = append(remaining, tc.Tool) + } + assert.Equal(t, tt.wantRemains, remaining) + }) + } +} + func TestTeardown_NoPurge_LeavesTokenRefsIntact(t *testing.T) { t.Parallel() From 1b089390542087237d81d237c396210c10219458 Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Thu, 13 Aug 2026 09:50:53 -0700 Subject: [PATCH 2/3] Clear client-scoped LLM settings stranded by teardown Bedrock compat was not the only persisted setting that could outlive the client it configures. Models has the same shape: it is read only by Claude Desktop and, under Bedrock compat, Claude Code, yet it survived a full teardown and was silently re-applied by the next setup. Replace the Bedrock-specific check with a registry of client-scoped settings, each naming the clients that consume it. Teardown clears any entry left with no consumer among the still-configured tools and reports what it removed, so a later setup no longer re-applies settings the user tore down. A setting is preserved while any of its consumers remains, so Models outlives a Claude Code teardown when Claude Desktop is still set up. Settings that apply to every client, such as TLSSkipVerify, are not registered and are unaffected. --- pkg/llm/config.go | 9 ++-- pkg/llm/setup.go | 96 ++++++++++++++++++++++++++++++++++++------- pkg/llm/setup_test.go | 87 ++++++++++++++++++++++++++------------- 3 files changed, 147 insertions(+), 45 deletions(-) diff --git a/pkg/llm/config.go b/pkg/llm/config.go index 82a7c4ca60..a91a01bcad 100644 --- a/pkg/llm/config.go +++ b/pkg/llm/config.go @@ -35,7 +35,9 @@ type Config struct { // Desktop) write it verbatim as inferenceModels, and — when Bedrock compat is // on — each entry is also mapped to a Claude Code tier (see BedrockConfig). // Persisting it here (rather than passing a transient flag value) keeps both - // consumers consistent on a later plain "thv llm setup". + // consumers consistent on a later plain "thv llm setup". Since only those two + // clients read it, it is registered in clientScopedSettings and cleared once + // neither remains configured. Models []string `yaml:"models,omitempty" json:"models,omitempty"` ConfiguredTools []ToolConfig `yaml:"configured_tools,omitempty" json:"configured_tools,omitempty"` } @@ -43,8 +45,9 @@ type Config struct { // BedrockConfig holds settings for configuring Claude Code to reach an LLM // gateway that forwards to AWS Bedrock. It is persisted so that a later plain // "thv llm setup" re-applies these settings rather than silently clearing them. -// Because it only ever applies to Claude Code, tearing Claude Code down clears -// it (see Teardown) so it cannot outlive its only consumer. +// Because it only ever applies to Claude Code, it is registered in +// clientScopedSettings and cleared when Claude Code is torn down, so it cannot +// outlive its only consumer. type BedrockConfig struct { // Compat, when true, writes CLAUDE_CODE_DISABLE_EXPERIMENTAL_BETAS=1 and the // per-tier Bedrock model IDs into Claude Code's settings.json. Bedrock rejects diff --git a/pkg/llm/setup.go b/pkg/llm/setup.go index d547da4d06..ed6b7e104f 100644 --- a/pkg/llm/setup.go +++ b/pkg/llm/setup.go @@ -11,6 +11,7 @@ import ( "net/http" "net/url" "os" + "slices" "strings" "time" @@ -191,8 +192,9 @@ func Setup( // configured tools. An error is returned when targetTool is non-empty but not // found in the configured tool list. // -// Reverting Claude Code also clears the persisted Bedrock compat settings, which -// apply to no other client, so a later "thv llm setup" does not re-apply them. +// Persisted settings that only apply to specific clients (see +// clientScopedSettings) are cleared once the last client that consumes them is +// reverted, so a later "thv llm setup" does not silently re-apply them. // // If secretsProvider is non-nil and purgeTokens is true, cached OIDC tokens // are deleted after the config update succeeds. @@ -242,16 +244,10 @@ func Teardown( // Persist the updated tool list (and clear token metadata if purging) in a // single write before mutating any tool config files. If this fails, // nothing on disk has changed and the caller can retry. + var cleared []string if err := provider.UpdateLLMConfig(func(c *Config) error { c.ConfiguredTools = remaining - // Bedrock compat only ever applies to Claude Code, so tearing Claude Code - // down leaves it with no consumer. Clear it rather than letting it persist: - // otherwise a later "thv llm setup" — possibly against a gateway that no - // longer forwards to Bedrock — silently re-pins the Bedrock model IDs the - // user just removed, and only an explicit --bedrock-compat=false clears it. - if isTarget(toRevert, claudeCodeClient) { - c.Bedrock = BedrockConfig{} - } + cleared = clearStrandedSettings(c, remaining) if purgeTokens { c.OIDC.CachedRefreshTokenRef = "" c.OIDC.CachedTokenExpiry = time.Time{} @@ -261,6 +257,12 @@ func Teardown( return fmt.Errorf("persisting tool configuration: %w", err) } + // Tell the user which persisted settings went away with the tools that used + // them, so a later setup that no longer applies them is not a surprise. + for _, name := range cleared { + _, _ = fmt.Fprintf(out, "Cleared the persisted %s: no configured tool uses it anymore.\n", name) + } + // Revert tool config files best-effort; warn on failure but do not undo // the config update above (the user can re-run setup+teardown to reconcile). for _, tc := range toRevert { @@ -368,10 +370,76 @@ func filterDetectedClients(detected []string, targetClient string) ([]string, er return nil, fmt.Errorf("client %q is not installed or not detected", targetClient) } -// claudeCodeClient is the canonical client identifier for Claude Code. Declared -// here as a string literal because pkg/llm does not import pkg/client (which -// owns the ClientApp constant) to avoid an import cycle. -const claudeCodeClient = "claude-code" +// Canonical client identifiers. Declared here as string literals because +// pkg/llm does not import pkg/client (which owns the ClientApp constants) to +// avoid an import cycle. +const ( + claudeCodeClient = "claude-code" + claudeDesktopClient = "claude-desktop" +) + +// clientScopedSetting describes a persisted setting that is only ever applied to +// a known subset of clients. Such a setting is deliberately sticky so an ordinary +// re-run of "thv llm setup" keeps it, but that stickiness becomes a bug once the +// last client that consumes it is torn down: the value survives in config.yaml +// with nothing to apply it to, and the next setup silently re-applies it — even +// against a gateway the user has since repointed elsewhere. Clearing it on +// teardown keeps "sticky across re-runs" without "immortal across teardowns". +type clientScopedSetting struct { + // name identifies the setting in the teardown notice shown to the user. + name string + // consumers lists the clients that apply this setting. The setting is cleared + // once none of them remain configured. + consumers []string + // isSet reports whether the setting currently holds a value worth clearing, + // so teardown stays silent for settings the user never set. + isSet func(*Config) bool + // clear resets the setting to its zero value. + clear func(*Config) +} + +// clientScopedSettings is the registry of settings that must not outlive their +// consumers. Add an entry here when introducing a persisted setting that applies +// to only some clients; a setting consumed by every client (e.g. TLSSkipVerify, +// which rides the shared gateway connection) does not belong here. +var clientScopedSettings = []clientScopedSetting{ + { + // Bedrock compat writes CLAUDE_CODE_DISABLE_EXPERIMENTAL_BETAS and the + // per-tier Bedrock model IDs, and is read only by the Claude Code path. + name: "Bedrock compatibility", + consumers: []string{claudeCodeClient}, + isSet: func(c *Config) bool { return c.Bedrock != BedrockConfig{} }, + clear: func(c *Config) { c.Bedrock = BedrockConfig{} }, + }, + { + // Models feeds Claude Desktop's inferenceModels and, under Bedrock compat, + // Claude Code's per-tier mapping. No other client reads it. + name: "model list", + consumers: []string{claudeDesktopClient, claudeCodeClient}, + isSet: func(c *Config) bool { return len(c.Models) > 0 }, + clear: func(c *Config) { c.Models = nil }, + }, +} + +// clearStrandedSettings zeroes every client-scoped setting left with no consumer +// among the still-configured tools. It returns the names of the settings it +// cleared so the caller can tell the user what was removed. +// +// It is called with the post-teardown tool list, so a setting is preserved as +// long as any client that reads it is still configured. +func clearStrandedSettings(c *Config, remaining []ToolConfig) []string { + var cleared []string + for _, s := range clientScopedSettings { + if !s.isSet(c) || slices.ContainsFunc(s.consumers, func(tool string) bool { + return isTarget(remaining, tool) + }) { + continue + } + s.clear(c) + cleared = append(cleared, s.name) + } + return cleared +} // Default Bedrock inference-profile model IDs written for Claude Code in // bedrock-compat mode when --models does not override a tier. These track the diff --git a/pkg/llm/setup_test.go b/pkg/llm/setup_test.go index 7a716718b1..ec4f68a92e 100644 --- a/pkg/llm/setup_test.go +++ b/pkg/llm/setup_test.go @@ -8,6 +8,7 @@ import ( "context" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -355,44 +356,54 @@ func TestTeardown_PurgeTokens_ClearsConfigRefsAndDeletesSecrets(t *testing.T) { assert.Equal(t, []string{"cursor"}, gm.reverted) } -// TestTeardown_BedrockClearedWithClaudeCode verifies that the persisted Bedrock -// settings are cleared exactly when Claude Code is reverted — they apply to no -// other client, so leaving them would let a later "thv llm setup" silently -// re-pin the Bedrock model IDs the user just tore down. -func TestTeardown_BedrockClearedWithClaudeCode(t *testing.T) { +// TestTeardown_ClearsStrandedClientScopedSettings verifies that a persisted +// client-scoped setting is cleared exactly when the last client that consumes it +// is reverted. Leaving one behind would let a later "thv llm setup" silently +// re-apply settings the user just tore down. Bedrock has a single consumer +// (Claude Code); Models has two (Claude Desktop and, under Bedrock compat, +// Claude Code), so it must survive until both are gone. +func TestTeardown_ClearsStrandedClientScopedSettings(t *testing.T) { t.Parallel() tests := []struct { name string - configured []ToolConfig + configured []string targetTool string - wantCompat bool + wantBedrock bool + wantModels bool wantRemains []string }{ { - name: "reverting claude-code clears bedrock", - configured: []ToolConfig{{Tool: "claude-code", ConfigPath: "/tmp/claude.json"}}, - targetTool: "claude-code", - wantCompat: false, + name: "reverting the only consumer clears both settings", + configured: []string{"claude-code"}, + targetTool: "claude-code", + wantBedrock: false, + wantModels: false, }, { - name: "reverting another tool keeps bedrock for claude-code", - configured: []ToolConfig{ - {Tool: "claude-code", ConfigPath: "/tmp/claude.json"}, - {Tool: "cursor", ConfigPath: "/tmp/cursor.json"}, - }, + name: "reverting an unrelated tool keeps both settings", + configured: []string{"claude-code", "cursor"}, targetTool: "cursor", - wantCompat: true, + wantBedrock: true, + wantModels: true, wantRemains: []string{"claude-code"}, }, { - name: "reverting all tools clears bedrock", - configured: []ToolConfig{ - {Tool: "claude-code", ConfigPath: "/tmp/claude.json"}, - {Tool: "cursor", ConfigPath: "/tmp/cursor.json"}, - }, - targetTool: "", - wantCompat: false, + // Models has a second consumer, so it must outlive Claude Code here + // while Bedrock — which only Claude Code reads — is cleared. + name: "second models consumer keeps models but not bedrock", + configured: []string{"claude-code", "claude-desktop"}, + targetTool: "claude-code", + wantBedrock: false, + wantModels: true, + wantRemains: []string{"claude-desktop"}, + }, + { + name: "reverting all tools clears both settings", + configured: []string{"claude-code", "claude-desktop", "cursor"}, + targetTool: "", + wantBedrock: false, + wantModels: false, }, } @@ -400,9 +411,14 @@ func TestTeardown_BedrockClearedWithClaudeCode(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() + configured := make([]ToolConfig, len(tt.configured)) + for i, tool := range tt.configured { + configured[i] = ToolConfig{Tool: tool, ConfigPath: "/tmp/" + tool + ".json"} + } provider := &stubConfigUpdater{cfg: Config{ - ConfiguredTools: tt.configured, + ConfiguredTools: configured, Bedrock: BedrockConfig{Compat: true, Enable1M: true}, + Models: []string{"us.anthropic.claude-opus-4-8"}, }} var stdout, stderr bytes.Buffer @@ -410,9 +426,15 @@ func TestTeardown_BedrockClearedWithClaudeCode(t *testing.T) { &stubGatewayManager{}, tt.targetTool, false, provider, nil) require.NoError(t, err) - assert.Equal(t, tt.wantCompat, provider.cfg.Bedrock.Compat) - // Enable1M rides the same config and must not outlive Compat. - assert.Equal(t, tt.wantCompat, provider.cfg.Bedrock.Enable1M) + assert.Equal(t, tt.wantBedrock, provider.cfg.Bedrock.Compat) + // Enable1M rides the same struct and must not outlive Compat. + assert.Equal(t, tt.wantBedrock, provider.cfg.Bedrock.Enable1M) + assert.Equal(t, tt.wantModels, len(provider.cfg.Models) > 0) + + // A cleared setting must be reported so the user is not surprised when + // a later setup no longer applies it. + assert.Equal(t, !tt.wantBedrock, strings.Contains(stdout.String(), "Bedrock compatibility")) + assert.Equal(t, !tt.wantModels, strings.Contains(stdout.String(), "model list")) var remaining []string for _, tc := range provider.cfg.ConfiguredTools { @@ -423,6 +445,15 @@ func TestTeardown_BedrockClearedWithClaudeCode(t *testing.T) { } } +// TestClearStrandedSettings_SilentWhenUnset verifies that teardown reports +// nothing for settings the user never set, so the notice stays signal. +func TestClearStrandedSettings_SilentWhenUnset(t *testing.T) { + t.Parallel() + + cfg := &Config{} + assert.Empty(t, clearStrandedSettings(cfg, nil)) +} + func TestTeardown_NoPurge_LeavesTokenRefsIntact(t *testing.T) { t.Parallel() From 2575cf5b9d11f1af4f2de10452e81847dae1fe40 Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Thu, 13 Aug 2026 11:01:24 -0700 Subject: [PATCH 3/3] Reset the LLM config when the last tool is torn down Replace the per-setting stranded-value pruning with a single rule: teardown resets the whole LLM config once no configured tool remains. Settings like Bedrock compat and the model list are deliberately sticky so an ordinary "thv llm setup" re-run keeps them. Nothing ended that stickiness, so a value could outlive every tool that read it and be silently re-applied by the next setup, possibly against a gateway the user had since repointed elsewhere. Resetting wholesale drops the need to track which client consumes which setting, and matches what "thv llm config reset" already does. A targeted teardown that leaves other tools configured keeps the config, which those tools still need to reach the gateway. Cached token state survives the reset unless --purge-tokens is passed: the secret lives in the keyring, and dropping the only reference without deleting it would strand it once the gateway URL changes. --- pkg/llm/config.go | 9 +-- pkg/llm/setup.go | 133 +++++++++++++----------------------- pkg/llm/setup_test.go | 155 +++++++++++++++++++++++------------------- 3 files changed, 135 insertions(+), 162 deletions(-) diff --git a/pkg/llm/config.go b/pkg/llm/config.go index a91a01bcad..8be3928bfc 100644 --- a/pkg/llm/config.go +++ b/pkg/llm/config.go @@ -35,9 +35,7 @@ type Config struct { // Desktop) write it verbatim as inferenceModels, and — when Bedrock compat is // on — each entry is also mapped to a Claude Code tier (see BedrockConfig). // Persisting it here (rather than passing a transient flag value) keeps both - // consumers consistent on a later plain "thv llm setup". Since only those two - // clients read it, it is registered in clientScopedSettings and cleared once - // neither remains configured. + // consumers consistent on a later plain "thv llm setup". Models []string `yaml:"models,omitempty" json:"models,omitempty"` ConfiguredTools []ToolConfig `yaml:"configured_tools,omitempty" json:"configured_tools,omitempty"` } @@ -45,9 +43,8 @@ type Config struct { // BedrockConfig holds settings for configuring Claude Code to reach an LLM // gateway that forwards to AWS Bedrock. It is persisted so that a later plain // "thv llm setup" re-applies these settings rather than silently clearing them. -// Because it only ever applies to Claude Code, it is registered in -// clientScopedSettings and cleared when Claude Code is torn down, so it cannot -// outlive its only consumer. +// That stickiness ends at teardown: reverting the last configured tool resets the +// whole LLM config (see Teardown), so it cannot outlive the tools that used it. type BedrockConfig struct { // Compat, when true, writes CLAUDE_CODE_DISABLE_EXPERIMENTAL_BETAS=1 and the // per-tier Bedrock model IDs into Claude Code's settings.json. Bedrock rejects diff --git a/pkg/llm/setup.go b/pkg/llm/setup.go index ed6b7e104f..8bd8ab9a7c 100644 --- a/pkg/llm/setup.go +++ b/pkg/llm/setup.go @@ -11,7 +11,6 @@ import ( "net/http" "net/url" "os" - "slices" "strings" "time" @@ -192,9 +191,11 @@ func Setup( // configured tools. An error is returned when targetTool is non-empty but not // found in the configured tool list. // -// Persisted settings that only apply to specific clients (see -// clientScopedSettings) are cleared once the last client that consumes them is -// reverted, so a later "thv llm setup" does not silently re-apply them. +// Reverting the last configured tool also resets the persisted LLM config to its +// zero value, so settings that are deliberately sticky across "thv llm setup" +// re-runs (e.g. Bedrock compat) are not silently re-applied by a later setup. +// A targeted teardown that leaves other tools configured keeps the config, which +// those tools still need to reach the gateway. // // If secretsProvider is non-nil and purgeTokens is true, cached OIDC tokens // are deleted after the config update succeeds. @@ -244,23 +245,17 @@ func Teardown( // Persist the updated tool list (and clear token metadata if purging) in a // single write before mutating any tool config files. If this fails, // nothing on disk has changed and the caller can retry. - var cleared []string + lastTool := len(remaining) == 0 if err := provider.UpdateLLMConfig(func(c *Config) error { - c.ConfiguredTools = remaining - cleared = clearStrandedSettings(c, remaining) - if purgeTokens { - c.OIDC.CachedRefreshTokenRef = "" - c.OIDC.CachedTokenExpiry = time.Time{} - } + applyTeardownToConfig(c, remaining, purgeTokens) return nil }); err != nil { return fmt.Errorf("persisting tool configuration: %w", err) } - // Tell the user which persisted settings went away with the tools that used - // them, so a later setup that no longer applies them is not a surprise. - for _, name := range cleared { - _, _ = fmt.Fprintf(out, "Cleared the persisted %s: no configured tool uses it anymore.\n", name) + if lastTool { + _, _ = fmt.Fprintln(out, + "Cleared the LLM gateway configuration: no tools are configured anymore.") } // Revert tool config files best-effort; warn on failure but do not undo @@ -278,6 +273,40 @@ func Teardown( return nil } +// applyTeardownToConfig updates the persisted LLM config for a teardown that +// leaves remaining configured. purgeTokens reports whether the caller also asked +// to drop cached OIDC token state. +// +// When no tool remains, the config is reset wholesale rather than pruned field by +// field. Settings such as Bedrock compat are deliberately sticky across "thv llm +// setup" re-runs, so keeping them past the last teardown would let the next setup +// silently re-apply settings the user just removed — possibly against a gateway +// they have since repointed elsewhere. While any tool remains the config is left +// intact, since those tools still need it to reach the gateway. +// +// Cached token state survives a reset unless purgeTokens is set: the secret lives +// in the keyring, and dropping the only reference to it without deleting it would +// strand it there once the user points at a different gateway (the fallback key is +// derived from the gateway URL and issuer). Token lifetime stays the exclusive +// business of --purge-tokens. +func applyTeardownToConfig(c *Config, remaining []ToolConfig, purgeTokens bool) { + if len(remaining) > 0 { + c.ConfiguredTools = remaining + if purgeTokens { + c.OIDC.CachedRefreshTokenRef = "" + c.OIDC.CachedTokenExpiry = time.Time{} + } + return + } + + tokenState := c.OIDC + *c = Config{} + if !purgeTokens { + c.OIDC.CachedRefreshTokenRef = tokenState.CachedRefreshTokenRef + c.OIDC.CachedTokenExpiry = tokenState.CachedTokenExpiry + } +} + // PurgeTokens deletes all cached OIDC tokens from the provided secrets // provider. Errors are logged as warnings rather than returned. func PurgeTokens(ctx context.Context, errOut io.Writer, provider pkgsecrets.Provider) { @@ -370,76 +399,10 @@ func filterDetectedClients(detected []string, targetClient string) ([]string, er return nil, fmt.Errorf("client %q is not installed or not detected", targetClient) } -// Canonical client identifiers. Declared here as string literals because -// pkg/llm does not import pkg/client (which owns the ClientApp constants) to -// avoid an import cycle. -const ( - claudeCodeClient = "claude-code" - claudeDesktopClient = "claude-desktop" -) - -// clientScopedSetting describes a persisted setting that is only ever applied to -// a known subset of clients. Such a setting is deliberately sticky so an ordinary -// re-run of "thv llm setup" keeps it, but that stickiness becomes a bug once the -// last client that consumes it is torn down: the value survives in config.yaml -// with nothing to apply it to, and the next setup silently re-applies it — even -// against a gateway the user has since repointed elsewhere. Clearing it on -// teardown keeps "sticky across re-runs" without "immortal across teardowns". -type clientScopedSetting struct { - // name identifies the setting in the teardown notice shown to the user. - name string - // consumers lists the clients that apply this setting. The setting is cleared - // once none of them remain configured. - consumers []string - // isSet reports whether the setting currently holds a value worth clearing, - // so teardown stays silent for settings the user never set. - isSet func(*Config) bool - // clear resets the setting to its zero value. - clear func(*Config) -} - -// clientScopedSettings is the registry of settings that must not outlive their -// consumers. Add an entry here when introducing a persisted setting that applies -// to only some clients; a setting consumed by every client (e.g. TLSSkipVerify, -// which rides the shared gateway connection) does not belong here. -var clientScopedSettings = []clientScopedSetting{ - { - // Bedrock compat writes CLAUDE_CODE_DISABLE_EXPERIMENTAL_BETAS and the - // per-tier Bedrock model IDs, and is read only by the Claude Code path. - name: "Bedrock compatibility", - consumers: []string{claudeCodeClient}, - isSet: func(c *Config) bool { return c.Bedrock != BedrockConfig{} }, - clear: func(c *Config) { c.Bedrock = BedrockConfig{} }, - }, - { - // Models feeds Claude Desktop's inferenceModels and, under Bedrock compat, - // Claude Code's per-tier mapping. No other client reads it. - name: "model list", - consumers: []string{claudeDesktopClient, claudeCodeClient}, - isSet: func(c *Config) bool { return len(c.Models) > 0 }, - clear: func(c *Config) { c.Models = nil }, - }, -} - -// clearStrandedSettings zeroes every client-scoped setting left with no consumer -// among the still-configured tools. It returns the names of the settings it -// cleared so the caller can tell the user what was removed. -// -// It is called with the post-teardown tool list, so a setting is preserved as -// long as any client that reads it is still configured. -func clearStrandedSettings(c *Config, remaining []ToolConfig) []string { - var cleared []string - for _, s := range clientScopedSettings { - if !s.isSet(c) || slices.ContainsFunc(s.consumers, func(tool string) bool { - return isTarget(remaining, tool) - }) { - continue - } - s.clear(c) - cleared = append(cleared, s.name) - } - return cleared -} +// claudeCodeClient is the canonical client identifier for Claude Code. Declared +// here as a string literal because pkg/llm does not import pkg/client (which +// owns the ClientApp constant) to avoid an import cycle. +const claudeCodeClient = "claude-code" // Default Bedrock inference-profile model IDs written for Claude Code in // bedrock-compat mode when --models does not override a tier. These track the diff --git a/pkg/llm/setup_test.go b/pkg/llm/setup_test.go index ec4f68a92e..141d3a5f4b 100644 --- a/pkg/llm/setup_test.go +++ b/pkg/llm/setup_test.go @@ -8,7 +8,6 @@ import ( "context" "net/http" "net/http/httptest" - "strings" "testing" "time" @@ -356,54 +355,52 @@ func TestTeardown_PurgeTokens_ClearsConfigRefsAndDeletesSecrets(t *testing.T) { assert.Equal(t, []string{"cursor"}, gm.reverted) } -// TestTeardown_ClearsStrandedClientScopedSettings verifies that a persisted -// client-scoped setting is cleared exactly when the last client that consumes it -// is reverted. Leaving one behind would let a later "thv llm setup" silently -// re-apply settings the user just tore down. Bedrock has a single consumer -// (Claude Code); Models has two (Claude Desktop and, under Bedrock compat, -// Claude Code), so it must survive until both are gone. -func TestTeardown_ClearsStrandedClientScopedSettings(t *testing.T) { +// fullSetupConfig returns a config as "thv llm setup" would leave it, with the +// given tools configured and every persisted setting populated. +func fullSetupConfig(tools ...string) Config { + configured := make([]ToolConfig, len(tools)) + for i, tool := range tools { + configured[i] = ToolConfig{Tool: tool, ConfigPath: "/tmp/" + tool + ".json"} + } + return Config{ + GatewayURL: "https://gw.example.com", + TLSSkipVerify: true, + OIDC: OIDCConfig{ + Issuer: "https://auth.example.com", + ClientID: "cid", + CachedRefreshTokenRef: "secret-ref", + }, + Proxy: ProxyConfig{ListenPort: 14001}, + Bedrock: BedrockConfig{Compat: true, Enable1M: true}, + Models: []string{"us.anthropic.claude-opus-4-8"}, + ConfiguredTools: configured, + } +} + +// TestTeardown_ResetsConfigWhenLastToolReverted verifies that reverting the last +// configured tool resets the whole LLM config. Settings like Bedrock compat are +// deliberately sticky across "thv llm setup" re-runs, so leaving them behind +// would let a later setup silently re-apply settings the user just tore down. +// +// Cached token state is the one exception: it is carried over so the keyring +// secret it points at is not stranded, and stays the business of --purge-tokens. +func TestTeardown_ResetsConfigWhenLastToolReverted(t *testing.T) { t.Parallel() tests := []struct { - name string - configured []string - targetTool string - wantBedrock bool - wantModels bool - wantRemains []string + name string + configured []string + targetTool string }{ { - name: "reverting the only consumer clears both settings", - configured: []string{"claude-code"}, - targetTool: "claude-code", - wantBedrock: false, - wantModels: false, - }, - { - name: "reverting an unrelated tool keeps both settings", - configured: []string{"claude-code", "cursor"}, - targetTool: "cursor", - wantBedrock: true, - wantModels: true, - wantRemains: []string{"claude-code"}, - }, - { - // Models has a second consumer, so it must outlive Claude Code here - // while Bedrock — which only Claude Code reads — is cleared. - name: "second models consumer keeps models but not bedrock", - configured: []string{"claude-code", "claude-desktop"}, - targetTool: "claude-code", - wantBedrock: false, - wantModels: true, - wantRemains: []string{"claude-desktop"}, + name: "targeted teardown of the only tool", + configured: []string{"claude-code"}, + targetTool: "claude-code", }, { - name: "reverting all tools clears both settings", - configured: []string{"claude-code", "claude-desktop", "cursor"}, - targetTool: "", - wantBedrock: false, - wantModels: false, + name: "untargeted teardown of every tool", + configured: []string{"claude-code", "claude-desktop", "cursor"}, + targetTool: "", }, } @@ -411,47 +408,63 @@ func TestTeardown_ClearsStrandedClientScopedSettings(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - configured := make([]ToolConfig, len(tt.configured)) - for i, tool := range tt.configured { - configured[i] = ToolConfig{Tool: tool, ConfigPath: "/tmp/" + tool + ".json"} - } - provider := &stubConfigUpdater{cfg: Config{ - ConfiguredTools: configured, - Bedrock: BedrockConfig{Compat: true, Enable1M: true}, - Models: []string{"us.anthropic.claude-opus-4-8"}, - }} + provider := &stubConfigUpdater{cfg: fullSetupConfig(tt.configured...)} var stdout, stderr bytes.Buffer err := Teardown(context.Background(), &stdout, &stderr, &stubGatewayManager{}, tt.targetTool, false, provider, nil) require.NoError(t, err) - assert.Equal(t, tt.wantBedrock, provider.cfg.Bedrock.Compat) - // Enable1M rides the same struct and must not outlive Compat. - assert.Equal(t, tt.wantBedrock, provider.cfg.Bedrock.Enable1M) - assert.Equal(t, tt.wantModels, len(provider.cfg.Models) > 0) - - // A cleared setting must be reported so the user is not surprised when - // a later setup no longer applies it. - assert.Equal(t, !tt.wantBedrock, strings.Contains(stdout.String(), "Bedrock compatibility")) - assert.Equal(t, !tt.wantModels, strings.Contains(stdout.String(), "model list")) - - var remaining []string - for _, tc := range provider.cfg.ConfiguredTools { - remaining = append(remaining, tc.Tool) - } - assert.Equal(t, tt.wantRemains, remaining) + want := Config{OIDC: OIDCConfig{CachedRefreshTokenRef: "secret-ref"}} + assert.Equal(t, want, provider.cfg, + "no tools remain, so every persisted setting except cached token state must be reset") + assert.Contains(t, stdout.String(), "Cleared the LLM gateway configuration") }) } } -// TestClearStrandedSettings_SilentWhenUnset verifies that teardown reports -// nothing for settings the user never set, so the notice stays signal. -func TestClearStrandedSettings_SilentWhenUnset(t *testing.T) { +// TestTeardown_ResetsCachedTokenStateWhenPurging verifies that --purge-tokens +// still clears the cached token refs on a full teardown, so the config keeps no +// pointer to secrets that PurgeTokens deletes. +func TestTeardown_ResetsCachedTokenStateWhenPurging(t *testing.T) { t.Parallel() - cfg := &Config{} - assert.Empty(t, clearStrandedSettings(cfg, nil)) + provider := &stubConfigUpdater{cfg: fullSetupConfig("claude-code")} + + var stdout, stderr bytes.Buffer + err := Teardown(context.Background(), &stdout, &stderr, + &stubGatewayManager{}, "", true, provider, nil) + require.NoError(t, err) + + assert.Equal(t, Config{}, provider.cfg) +} + +// TestTeardown_KeepsConfigWhileToolsRemain verifies that a targeted teardown +// preserves the gateway configuration the still-configured tools depend on. +// Zeroing it here would break "thv llm token" and "thv llm proxy start" for a +// tool the user never asked to touch, since both gate on IsConfigured(). +func TestTeardown_KeepsConfigWhileToolsRemain(t *testing.T) { + t.Parallel() + + provider := &stubConfigUpdater{cfg: fullSetupConfig("claude-code", "cursor")} + + var stdout, stderr bytes.Buffer + err := Teardown(context.Background(), &stdout, &stderr, + &stubGatewayManager{}, "claude-code", false, provider, nil) + require.NoError(t, err) + + assert.True(t, provider.cfg.IsConfigured(), + "cursor still routes through the gateway, so its config must survive") + assert.Equal(t, "secret-ref", provider.cfg.OIDC.CachedRefreshTokenRef, + "an unrelated teardown must not force a fresh login") + assert.Equal(t, 14001, provider.cfg.EffectiveProxyPort()) + assert.NotContains(t, stdout.String(), "Cleared the LLM gateway configuration") + + var remaining []string + for _, tc := range provider.cfg.ConfiguredTools { + remaining = append(remaining, tc.Tool) + } + assert.Equal(t, []string{"cursor"}, remaining) } func TestTeardown_NoPurge_LeavesTokenRefsIntact(t *testing.T) {