From fed7134f0a07bcf40fcb721a69a957de6559eeb8 Mon Sep 17 00:00:00 2001 From: taskbot Date: Thu, 30 Apr 2026 16:05:31 +0200 Subject: [PATCH 1/2] Correct Gemini CLI LLM gateway config to proxy mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gemini CLI has no dynamic token-command equivalent (no apiKeyHelper analogue), so it must use proxy mode like Cursor rather than direct mode. The previous config used two invalid settings.json keys: - /auth/tokenCommand — not a recognised Gemini CLI field - /baseUrl — silently ignored as a top-level key Replace with the correct proxy-mode keys: - /security/auth/selectedType = "gemini-api-key" (required so that GOOGLE_GEMINI_BASE_URL is honoured; OAuth auth ignores the override, fixed in gemini-cli v0.40.0 PR #25357) - /env/GEMINI_API_KEY = placeholder key (proxy does not validate it) - /env/GOOGLE_GEMINI_BASE_URL = local proxy address Fixes #5141 --- pkg/client/config.go | 39 ++++++++++---- pkg/client/llm_gateway.go | 7 +++ pkg/client/llm_gateway_test.go | 78 +++++++++++++++++++++++++++- pkg/llmgateway/config.go | 21 ++++++++ test/e2e/cli_llm_all_clients_test.go | 23 +++++--- 5 files changed, 148 insertions(+), 20 deletions(-) diff --git a/pkg/client/config.go b/pkg/client/config.go index 2e412fe732..2d2af38f97 100644 --- a/pkg/client/config.go +++ b/pkg/client/config.go @@ -161,14 +161,20 @@ const ( // literals by hujson.Patch. // ValueField names which LLMApplyConfig field to write: "GatewayURL", // "ProxyBaseURL", "TokenHelperCommand", "PlaceholderAPIKey" (constant "thv-proxy"), -// or "NodeTLSRejectUnauthorized" (writes "0" when TLSSkipVerify is true). +// "NodeTLSRejectUnauthorized" (writes "0" when TLSSkipVerify is true), or +// "ProxyOrigin" (like ProxyBaseURL but with the path stripped — use for tools +// such as Gemini CLI that append their own API path to avoid doubled segments). +// Any other string is written as a literal value; use this to hard-code a +// constant string into a settings key (e.g. forcing an auth type to a fixed value). // ClearWhenEmpty: when true and the resolved value is empty, the key is removed // from the settings file rather than skipped. Use for conditional keys like // NODE_TLS_REJECT_UNAUTHORIZED that must be cleaned up when the flag is cleared. type LLMGatewayKeySpec struct { - JSONPointer string // RFC 6901 path - ValueField string // "GatewayURL" | "ProxyBaseURL" | "TokenHelperCommand" | "PlaceholderAPIKey" | "NodeTLSRejectUnauthorized" - ClearWhenEmpty bool // remove the key when the resolved value is empty + JSONPointer string // RFC 6901 path + // ValueField: "GatewayURL" | "ProxyBaseURL" | "ProxyOrigin" | + // "TokenHelperCommand" | "PlaceholderAPIKey" | "NodeTLSRejectUnauthorized" + ValueField string + ClearWhenEmpty bool // remove the key when the resolved value is empty } // clientAppConfig represents a configuration path for a supported MCP client. @@ -839,17 +845,28 @@ var supportedClientIntegrations = []clientAppConfig{ SupportsSkills: true, SkillsGlobalPath: []string{".agents", skillsDirName}, SkillsProjectPath: []string{".agents", skillsDirName}, - // LLM gateway: patches the same settings.json used for MCP - LLMGatewayMode: "direct", + // LLM gateway: patches the same settings.json used for MCP. + // Gemini CLI has no dynamic token-command equivalent, so it uses the + // proxy path. GOOGLE_GEMINI_BASE_URL is only honoured when + // security.auth.selectedType is "gemini-api-key" (fixed in + // gemini-cli v0.40.0, PR #25357); OAuth auth ignores the override. + // + // NODE_TLS_REJECT_UNAUTHORIZED is intentionally omitted: in proxy mode + // the tool connects to the local proxy over plain HTTP, so there is no + // TLS handshake on that leg. Setting the env var would globally disable + // TLS verification for all other HTTPS requests the Gemini CLI process + // makes, which is an unacceptable side-effect. + LLMGatewayMode: "proxy", LLMBinaryName: "gemini", LLMSettingsFile: "settings.json", LLMSettingsRelPath: []string{".gemini"}, LLMGatewayKeys: []LLMGatewayKeySpec{ - {JSONPointer: "/auth/tokenCommand", ValueField: "TokenHelperCommand"}, - {JSONPointer: "/baseUrl", ValueField: "GatewayURL"}, - // NODE_TLS_REJECT_UNAUTHORIZED is only written when --tls-skip-verify is set. - // ClearWhenEmpty ensures it is removed when the flag is later cleared. - {JSONPointer: "/env/NODE_TLS_REJECT_UNAUTHORIZED", ValueField: "NodeTLSRejectUnauthorized", ClearWhenEmpty: true}, + // Force API-key auth so GOOGLE_GEMINI_BASE_URL is respected. + // "gemini-api-key" is an unrecognised ValueField, so llmValueForSpec + // writes it as a literal string (see LLMGatewayKeySpec doc comment). + {JSONPointer: "/security/auth/selectedType", ValueField: "gemini-api-key"}, + {JSONPointer: "/env/GEMINI_API_KEY", ValueField: "PlaceholderAPIKey"}, + {JSONPointer: "/env/GOOGLE_GEMINI_BASE_URL", ValueField: "ProxyOrigin"}, }, }, { diff --git a/pkg/client/llm_gateway.go b/pkg/client/llm_gateway.go index b0b6052675..24a0954853 100644 --- a/pkg/client/llm_gateway.go +++ b/pkg/client/llm_gateway.go @@ -261,6 +261,10 @@ func (cm *ClientManager) buildLLMSettingsPath(cfg *clientAppConfig) string { // llmValueForSpec returns the config value corresponding to the ValueField name. // For "NodeTLSRejectUnauthorized", returns "0" when TLSSkipVerify is true, or "" // when false (which triggers removal when ClearWhenEmpty is set on the spec). +// For "ProxyOrigin", returns ProxyBaseURL with the path stripped (origin only), +// for tools like Gemini CLI that append their own API path to avoid doubled segments. +// Any unrecognised ValueField string is returned as-is, allowing a spec entry to +// hard-code a constant value into a settings key (e.g. forcing an auth type). func llmValueForSpec(valueField string, cfg llmgateway.ApplyConfig) string { switch valueField { case "GatewayURL": @@ -276,6 +280,9 @@ func llmValueForSpec(valueField string, cfg llmgateway.ApplyConfig) string { return "0" } return "" + case "ProxyOrigin": + // Like ProxyBaseURL but with the path stripped; see llmgateway.ProxyOriginOf. + return llmgateway.ProxyOriginOf(cfg.ProxyBaseURL) default: return valueField // treat unknown field names as literal values } diff --git a/pkg/client/llm_gateway_test.go b/pkg/client/llm_gateway_test.go index a1e64e2ca9..f8ee5dbeb0 100644 --- a/pkg/client/llm_gateway_test.go +++ b/pkg/client/llm_gateway_test.go @@ -57,9 +57,12 @@ func TestRealClientConfigs_ConfigureAndRevert(t *testing.T) { }, { // ~/.gemini/settings.json + // NODE_TLS_REJECT_UNAUTHORIZED must NOT appear when TLSSkipVerify is false. clientType: GeminiCli, expectedKeys: []string{ - "tokenCommand", "baseUrl", "https://gw.example.com", + "selectedType", "gemini-api-key", + "GEMINI_API_KEY", "thv-proxy", + "GOOGLE_GEMINI_BASE_URL", "http://localhost:14000", }, }, { @@ -624,6 +627,79 @@ func TestConfigureLLMGateway_TLSSkipVerify_ClearRemovesKey(t *testing.T) { assert.NotContains(t, string(data), "NODE_TLS_REJECT_UNAUTHORIZED", "key must be removed when TLSSkipVerify is cleared") } +// TestRealClientConfigs_GeminiCLI_NeverWritesTLSKey verifies that +// NODE_TLS_REJECT_UNAUTHORIZED is never written for Gemini CLI regardless of +// TLSSkipVerify. In proxy mode the tool connects to localhost over plain HTTP, +// so setting the env var would only globally suppress TLS for other HTTPS +// requests — an unacceptable side-effect. The key spec is intentionally absent. +func TestRealClientConfigs_GeminiCLI_NeverWritesTLSKey(t *testing.T) { + t.Parallel() + + home := t.TempDir() + cm := NewTestClientManager(home, nil, supportedClientIntegrations, nil) + require.NoError(t, os.MkdirAll(filepath.Join(home, ".gemini"), 0o700)) + + for _, skipVerify := range []bool{false, true} { + path, err := cm.ConfigureLLMGateway(GeminiCli, llmgateway.ApplyConfig{ + ProxyBaseURL: "http://localhost:14000/v1", + TLSSkipVerify: skipVerify, + }) + require.NoError(t, err) + + data, err := os.ReadFile(path) + require.NoError(t, err) + assert.NotContains(t, string(data), "NODE_TLS_REJECT_UNAUTHORIZED", + "TLS key must never be written for Gemini CLI (TLSSkipVerify=%v)", skipVerify) + } +} + +// ── llmValueForSpec unit tests ──────────────────────────────────────────────── + +func TestLLMValueForSpec(t *testing.T) { + t.Parallel() + + cfg := llmgateway.ApplyConfig{ + GatewayURL: "https://gw.example.com", + ProxyBaseURL: "http://localhost:14000/v1", + TokenHelperCommand: `"thv" llm token`, + TLSSkipVerify: false, + } + + cases := []struct { + name string + valueField string + cfg llmgateway.ApplyConfig + want string + }{ + {"GatewayURL", "GatewayURL", cfg, "https://gw.example.com"}, + {"ProxyBaseURL", "ProxyBaseURL", cfg, "http://localhost:14000/v1"}, + {"TokenHelperCommand", "TokenHelperCommand", cfg, `"thv" llm token`}, + {"PlaceholderAPIKey", "PlaceholderAPIKey", cfg, "thv-proxy"}, + // NodeTLSRejectUnauthorized: "0" when set, "" when clear + {"NodeTLSRejectUnauthorized/skip=false", "NodeTLSRejectUnauthorized", cfg, ""}, + {"NodeTLSRejectUnauthorized/skip=true", "NodeTLSRejectUnauthorized", llmgateway.ApplyConfig{TLSSkipVerify: true}, "0"}, + // ProxyOrigin strips path, query, and fragment from ProxyBaseURL + {"ProxyOrigin/strips_path", "ProxyOrigin", cfg, "http://localhost:14000"}, + {"ProxyOrigin/long_path", "ProxyOrigin", llmgateway.ApplyConfig{ProxyBaseURL: "http://localhost:9000/v1beta/openai"}, "http://localhost:9000"}, + {"ProxyOrigin/strips_query_and_fragment", "ProxyOrigin", llmgateway.ApplyConfig{ProxyBaseURL: "http://host:8080/path?q=1#frag"}, "http://host:8080"}, + // ForceQuery: trailing "?" with no key must not leak into the origin. + {"ProxyOrigin/force_query", "ProxyOrigin", llmgateway.ApplyConfig{ProxyBaseURL: "http://host:8080/path?"}, "http://host:8080"}, + // ProxyOrigin falls back to the raw value when URL parsing fails + {"ProxyOrigin/invalid_url_fallback", "ProxyOrigin", llmgateway.ApplyConfig{ProxyBaseURL: "::invalid"}, "::invalid"}, + // Unrecognised ValueField is written as a literal constant + {"literal/gemini-api-key", "gemini-api-key", cfg, "gemini-api-key"}, + {"literal/some-literal", "some-literal", cfg, "some-literal"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := llmValueForSpec(tc.valueField, tc.cfg) + assert.Equal(t, tc.want, got) + }) + } +} + // countSubstring counts non-overlapping occurrences of substr in s. func countSubstring(s, substr string) int { count := 0 diff --git a/pkg/llmgateway/config.go b/pkg/llmgateway/config.go index 5de820147f..418cf392fd 100644 --- a/pkg/llmgateway/config.go +++ b/pkg/llmgateway/config.go @@ -5,6 +5,27 @@ // imported by both pkg/llm and pkg/client to avoid an import cycle. package llmgateway +import "net/url" + +// ProxyOriginOf returns rawURL with its path, query, and fragment stripped so +// only the scheme and host remain (the "origin"). Tools like Gemini CLI that +// append their own API path (e.g. /v1beta/...) need the origin rather than the +// full proxy base URL to avoid doubled path segments. Falls back to rawURL when +// parsing fails. +func ProxyOriginOf(rawURL string) string { + u, err := url.Parse(rawURL) + if err != nil || u == nil { + return rawURL + } + u.Path = "" + u.RawPath = "" + u.RawQuery = "" + u.ForceQuery = false + u.Fragment = "" + u.RawFragment = "" + return u.String() +} + // ApplyConfig holds the values needed to configure a single tool's LLM // gateway settings. Using a struct prevents positional-argument mistakes when // the caller has multiple similar string values in scope. diff --git a/test/e2e/cli_llm_all_clients_test.go b/test/e2e/cli_llm_all_clients_test.go index 3980dad4de..029d7d82a1 100644 --- a/test/e2e/cli_llm_all_clients_test.go +++ b/test/e2e/cli_llm_all_clients_test.go @@ -19,6 +19,7 @@ import ( "github.com/stacklok/toolhive/pkg/auth/tokensource" "github.com/stacklok/toolhive/pkg/llm" + "github.com/stacklok/toolhive/pkg/llmgateway" "github.com/stacklok/toolhive/pkg/networking" "github.com/stacklok/toolhive/test/e2e" ) @@ -89,11 +90,17 @@ func allClientTestCases() []llmClientTestCase { settingsPath: func(tempDir string) string { return filepath.Join(tempDir, ".gemini", "settings.json") }, - mode: "direct", + mode: "proxy", expectedKeys: map[string]func(string, string) string{ - "/auth/tokenCommand": func(_, _ string) string { return "llm token" }, - "/baseUrl": func(gatewayURL, _ string) string { - return gatewayURL + // Force API-key auth so GOOGLE_GEMINI_BASE_URL is respected. + "/security/auth/selectedType": func(_, _ string) string { return "gemini-api-key" }, + "/env/GEMINI_API_KEY": func(_, _ string) string { return clientThvProxy }, + // ProxyOrigin strips the entire path (and any query/fragment) from + // the proxy base URL so Gemini CLI can append its own /v1beta/... + // path without doubling segments. Mirror the production logic here + // so the expectation stays correct if the proxy path changes. + "/env/GOOGLE_GEMINI_BASE_URL": func(_, proxyURL string) string { + return llmgateway.ProxyOriginOf(proxyURL) }, }, }, @@ -448,11 +455,11 @@ var _ = Describe("thv llm — all-client matrix", Label("cli", "llm", "clients", Expect(err).ToNot(HaveOccurred()) var geminiAfter map[string]any Expect(json.Unmarshal(data, &geminiAfter)).To(Succeed()) - tokenCmd, found := jsonPointerGet(geminiAfter, "/auth/tokenCommand") + baseURL, found := jsonPointerGet(geminiAfter, "/env/GOOGLE_GEMINI_BASE_URL") Expect(found).To(BeTrue(), - "gemini-cli /auth/tokenCommand should still be present after claude-code teardown") - Expect(tokenCmd).To(ContainSubstring("llm token"), - "gemini-cli tokenCommand should still reference 'llm token' after claude-code teardown") + "gemini-cli /env/GOOGLE_GEMINI_BASE_URL should still be present after claude-code teardown") + Expect(baseURL).ToNot(BeEmpty(), + "gemini-cli GOOGLE_GEMINI_BASE_URL should still be set after claude-code teardown") By("verifying ConfiguredTools has only gemini-cli") showOut, _ = thvCmd("llm", "config", "show", "--format", "json").ExpectSuccess() From 29d70feef1f7557e279c3a1b54bf7896699f0f1d Mon Sep 17 00:00:00 2001 From: taskbot Date: Mon, 4 May 2026 08:46:55 +0200 Subject: [PATCH 2/2] fixes from review --- cmd/thv/app/llm.go | 4 + pkg/client/config.go | 52 ++++++++---- pkg/client/llm_gateway.go | 64 ++++++++++----- pkg/client/llm_gateway_test.go | 145 +++++++++++++++++++++------------ pkg/llm/setup.go | 6 ++ pkg/llm/setup_test.go | 1 + pkg/llmgateway/config.go | 30 ++++--- pkg/llmgateway/config_test.go | 50 ++++++++++++ 8 files changed, 251 insertions(+), 101 deletions(-) create mode 100644 pkg/llmgateway/config_test.go diff --git a/cmd/thv/app/llm.go b/cmd/thv/app/llm.go index 1cb806c4b4..92556b0c79 100644 --- a/cmd/thv/app/llm.go +++ b/cmd/thv/app/llm.go @@ -375,6 +375,10 @@ func (a *clientManagerAdapter) LLMGatewayModeFor(clientType string) string { return a.cm.LLMGatewayModeFor(client.ClientApp(clientType)) } +func (a *clientManagerAdapter) LLMSetupNoteFor(clientType string) string { + return a.cm.LLMSetupNoteFor(client.ClientApp(clientType)) +} + func (a *clientManagerAdapter) RevertLLMGateway(clientType, configPath string) error { return a.cm.RevertLLMGateway(client.ClientApp(clientType), configPath) } diff --git a/pkg/client/config.go b/pkg/client/config.go index 2d2af38f97..7675211119 100644 --- a/pkg/client/config.go +++ b/pkg/client/config.go @@ -159,22 +159,29 @@ const ( // path (e.g. "/apiKeyHelper" or "/env/ANTHROPIC_BASE_URL"). Dots in flat // top-level key names (e.g. "/cursor.general.openAIBaseURL") are treated as // literals by hujson.Patch. -// ValueField names which LLMApplyConfig field to write: "GatewayURL", -// "ProxyBaseURL", "TokenHelperCommand", "PlaceholderAPIKey" (constant "thv-proxy"), -// "NodeTLSRejectUnauthorized" (writes "0" when TLSSkipVerify is true), or -// "ProxyOrigin" (like ProxyBaseURL but with the path stripped — use for tools -// such as Gemini CLI that append their own API path to avoid doubled segments). -// Any other string is written as a literal value; use this to hard-code a -// constant string into a settings key (e.g. forcing an auth type to a fixed value). -// ClearWhenEmpty: when true and the resolved value is empty, the key is removed -// from the settings file rather than skipped. Use for conditional keys like -// NODE_TLS_REJECT_UNAUTHORIZED that must be cleaned up when the flag is cleared. +// +// Exactly one of ValueField or Literal must be set: +// - ValueField names which ApplyConfig field to write. Valid values: +// "GatewayURL", "ProxyBaseURL", "ProxyOrigin", "TokenHelperCommand", +// "PlaceholderAPIKey", "NodeTLSRejectUnauthorized". +// An unrecognised ValueField is a programming error and causes +// ConfigureLLMGateway to return an error. +// - Literal is written verbatim into the settings key (e.g. a fixed auth +// type string). Use Literal instead of ValueField for constant values so +// that typos in ValueField are caught as errors rather than silently +// written as unexpected strings. +// +// ClearWhenEmpty: when true and the resolved value is empty, the key is +// removed from the settings file rather than skipped. Use for conditional +// keys like NODE_TLS_REJECT_UNAUTHORIZED that must be cleaned up when the +// flag is cleared. Ignored when Literal is set (literals are never empty). type LLMGatewayKeySpec struct { JSONPointer string // RFC 6901 path // ValueField: "GatewayURL" | "ProxyBaseURL" | "ProxyOrigin" | // "TokenHelperCommand" | "PlaceholderAPIKey" | "NodeTLSRejectUnauthorized" ValueField string - ClearWhenEmpty bool // remove the key when the resolved value is empty + Literal string // constant value written verbatim; mutually exclusive with ValueField + ClearWhenEmpty bool // remove the key when the resolved value is empty (ignored for Literal) } // clientAppConfig represents a configuration path for a supported MCP client. @@ -229,6 +236,10 @@ type clientAppConfig struct { // LLMGatewayKeys lists the JSON Pointer paths and value-field mappings to // apply when setting up (or reverting) LLM gateway access. LLMGatewayKeys []LLMGatewayKeySpec + // LLMSetupNote is an optional message printed to stdout after a successful + // "thv llm setup" for this client. Use it to surface manual steps that + // toolhive cannot automate (e.g. env vars the tool reads from a .env file). + LLMSetupNote string } // extractServersKeyFromConfig extracts the servers key from MCPServersPathPrefix @@ -851,6 +862,11 @@ var supportedClientIntegrations = []clientAppConfig{ // security.auth.selectedType is "gemini-api-key" (fixed in // gemini-cli v0.40.0, PR #25357); OAuth auth ignores the override. // + // GOOGLE_GEMINI_BASE_URL and GEMINI_API_KEY are NOT written here because + // settings.json has no top-level env field (additionalProperties: false); + // Gemini CLI reads those variables from process.env, not from settings. + // Users must set them manually in ~/.gemini/.env (or OS environment). + // // NODE_TLS_REJECT_UNAUTHORIZED is intentionally omitted: in proxy mode // the tool connects to the local proxy over plain HTTP, so there is no // TLS handshake on that leg. Setting the env var would globally disable @@ -862,12 +878,14 @@ var supportedClientIntegrations = []clientAppConfig{ LLMSettingsRelPath: []string{".gemini"}, LLMGatewayKeys: []LLMGatewayKeySpec{ // Force API-key auth so GOOGLE_GEMINI_BASE_URL is respected. - // "gemini-api-key" is an unrecognised ValueField, so llmValueForSpec - // writes it as a literal string (see LLMGatewayKeySpec doc comment). - {JSONPointer: "/security/auth/selectedType", ValueField: "gemini-api-key"}, - {JSONPointer: "/env/GEMINI_API_KEY", ValueField: "PlaceholderAPIKey"}, - {JSONPointer: "/env/GOOGLE_GEMINI_BASE_URL", ValueField: "ProxyOrigin"}, - }, + {JSONPointer: "/security/auth/selectedType", Literal: "gemini-api-key"}, + }, + LLMSetupNote: "Note (gemini-cli): settings.json cannot inject env vars into the Gemini CLI process.\n" + + "To complete proxy setup, add the following to ~/.gemini/.env:\n" + + " GEMINI_API_KEY=thv-proxy\n" + + " GOOGLE_GEMINI_BASE_URL= # e.g. http://localhost:14000\n" + + "Full end-to-end proxy routing requires gateway support for the /v1beta/* path; " + + "track progress in the follow-up issue.", }, { ClientType: VSCodeServer, diff --git a/pkg/client/llm_gateway.go b/pkg/client/llm_gateway.go index 24a0954853..6ecba61037 100644 --- a/pkg/client/llm_gateway.go +++ b/pkg/client/llm_gateway.go @@ -80,9 +80,20 @@ func (cm *ClientManager) ConfigureLLMGateway(clientType ClientApp, cfg llmgatewa // allowing conditional keys (e.g. NODE_TLS_REJECT_UNAUTHORIZED) to be cleaned // up when the associated flag is cleared. func applyLLMGatewayKeys(v *hujson.Value, specs []LLMGatewayKeySpec, cfg llmgateway.ApplyConfig, filePath string) error { + // Resolve all values up front so an unknown ValueField fails fast before + // any file is modified. + values := make([]string, len(specs)) + for i, spec := range specs { + val, err := llmValueForSpec(spec, cfg) + if err != nil { + return err + } + values[i] = val + } + // Ensure ancestors only for specs that will be written (not removed). - for _, spec := range specs { - if spec.ClearWhenEmpty && llmValueForSpec(spec.ValueField, cfg) == "" { + for i, spec := range specs { + if spec.ClearWhenEmpty && values[i] == "" { continue } if err := ensureLLMAncestors(v, spec.JSONPointer, filePath); err != nil { @@ -96,8 +107,8 @@ func applyLLMGatewayKeys(v *hujson.Value, specs []LLMGatewayKeySpec, cfg llmgate return fmt.Errorf("standardizing %s: %w", filePath, err) } - for _, spec := range specs { - value := llmValueForSpec(spec.ValueField, cfg) + for i, spec := range specs { + value := values[i] if spec.ClearWhenEmpty && value == "" { if err := removeLLMKey(v, spec.JSONPointer, filePath, standardized); err != nil { return err @@ -219,6 +230,15 @@ func (cm *ClientManager) LLMGatewayModeFor(clientType ClientApp) string { return cfg.LLMGatewayMode } +// LLMSetupNoteFor returns the post-setup note for clientType, or "" if none. +func (cm *ClientManager) LLMSetupNoteFor(clientType ClientApp) string { + cfg := cm.lookupClientAppConfig(clientType) + if cfg == nil { + return "" + } + return cfg.LLMSetupNote +} + // DetectedLLMGatewayClients returns the subset of LLM-gateway-capable clients // that are installed on this machine. A client is considered installed when: // 1. Its settings directory exists on disk. @@ -258,33 +278,35 @@ func (cm *ClientManager) buildLLMSettingsPath(cfg *clientAppConfig) string { ) } -// llmValueForSpec returns the config value corresponding to the ValueField name. -// For "NodeTLSRejectUnauthorized", returns "0" when TLSSkipVerify is true, or "" -// when false (which triggers removal when ClearWhenEmpty is set on the spec). -// For "ProxyOrigin", returns ProxyBaseURL with the path stripped (origin only), -// for tools like Gemini CLI that append their own API path to avoid doubled segments. -// Any unrecognised ValueField string is returned as-is, allowing a spec entry to -// hard-code a constant value into a settings key (e.g. forcing an auth type). -func llmValueForSpec(valueField string, cfg llmgateway.ApplyConfig) string { - switch valueField { +// llmValueForSpec returns the value to write for spec given cfg. +// If spec.Literal is set it is returned directly. +// Otherwise spec.ValueField is resolved against cfg; an unrecognised ValueField +// returns an error so typos are caught at call time rather than silently written +// as unexpected strings into the user's settings file. +func llmValueForSpec(spec LLMGatewayKeySpec, cfg llmgateway.ApplyConfig) (string, error) { + if spec.Literal != "" { + return spec.Literal, nil + } + switch spec.ValueField { case "GatewayURL": - return cfg.GatewayURL + return cfg.GatewayURL, nil case "ProxyBaseURL": - return cfg.ProxyBaseURL + return cfg.ProxyBaseURL, nil case "TokenHelperCommand": - return cfg.TokenHelperCommand + return cfg.TokenHelperCommand, nil case "PlaceholderAPIKey": - return llmPlaceholderAPIKey + return llmPlaceholderAPIKey, nil case "NodeTLSRejectUnauthorized": if cfg.TLSSkipVerify { - return "0" + return "0", nil } - return "" + return "", nil case "ProxyOrigin": // Like ProxyBaseURL but with the path stripped; see llmgateway.ProxyOriginOf. - return llmgateway.ProxyOriginOf(cfg.ProxyBaseURL) + return llmgateway.ProxyOriginOf(cfg.ProxyBaseURL), nil default: - return valueField // treat unknown field names as literal values + return "", fmt.Errorf("unknown ValueField %q in LLMGatewayKeySpec for %q; use Literal for constant values", + spec.ValueField, spec.JSONPointer) } } diff --git a/pkg/client/llm_gateway_test.go b/pkg/client/llm_gateway_test.go index f8ee5dbeb0..4a474edb83 100644 --- a/pkg/client/llm_gateway_test.go +++ b/pkg/client/llm_gateway_test.go @@ -4,16 +4,46 @@ package client import ( + "encoding/json" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/tailscale/hujson" "github.com/stacklok/toolhive/pkg/llmgateway" ) +// jsonPointerGet resolves a JSON Pointer (RFC 6901) in data and returns the +// string value at that path, or ("", false) if the pointer does not exist or +// the value is not a string. data may be JSONC (hujson). +func jsonPointerGet(data []byte, pointer string) (string, bool) { + std, err := hujson.Standardize(data) + if err != nil { + return "", false + } + var root any + if err := json.Unmarshal(std, &root); err != nil { + return "", false + } + current := root + for _, seg := range strings.Split(strings.TrimPrefix(pointer, "/"), "/") { + m, ok := current.(map[string]any) + if !ok { + return "", false + } + current, ok = m[seg] + if !ok { + return "", false + } + } + s, ok := current.(string) + return s, ok +} + // fakeLLMBinary is the sentinel binary name used in tests that exercise the // exec.LookPath check inside DetectedLLMGatewayClients. The real lookup is // replaced by an injected stub, so no actual binary needs to exist. @@ -27,6 +57,10 @@ const fakeLLMBinary = "fake-llm-tool" // ValueField names, or missing intermediate-object creation in the real config // table — none of which are caught by tests that use fake clientAppConfig // entries via LLMTestIntegrations. +// +// Values are asserted via exact JSON-pointer lookups rather than raw substring +// checks, so a value landing at a wrong pointer (or as a stray key name) will +// fail the test rather than silently pass. func TestRealClientConfigs_ConfigureAndRevert(t *testing.T) { t.Parallel() @@ -40,61 +74,61 @@ func TestRealClientConfigs_ConfigureAndRevert(t *testing.T) { TokenHelperCommand: `"thv" llm token`, } - // expectedKeys lists substrings that must appear in the settings file after - // Configure, and must NOT appear after Revert. Each entry maps to one real - // clientAppConfig in supportedClientIntegrations. + // wantPointers maps RFC 6901 JSON pointer → expected string value after + // Configure. After Revert every pointer must be absent. cases := []struct { clientType ClientApp - expectedKeys []string + wantPointers map[string]string }{ { // ~/.claude/settings.json clientType: ClaudeCode, - expectedKeys: []string{ - "apiKeyHelper", - "ANTHROPIC_BASE_URL", "https://gw.example.com", + wantPointers: map[string]string{ + "/apiKeyHelper": `"thv" llm token`, + "/env/ANTHROPIC_BASE_URL": "https://gw.example.com", }, }, { // ~/.gemini/settings.json + // Only the auth type is patched; env vars are not written because + // settings.json has no top-level env field — they must be set + // manually in ~/.gemini/.env. // NODE_TLS_REJECT_UNAUTHORIZED must NOT appear when TLSSkipVerify is false. clientType: GeminiCli, - expectedKeys: []string{ - "selectedType", "gemini-api-key", - "GEMINI_API_KEY", "thv-proxy", - "GOOGLE_GEMINI_BASE_URL", "http://localhost:14000", + wantPointers: map[string]string{ + "/security/auth/selectedType": "gemini-api-key", }, }, { // ~/./Cursor/User/settings.json clientType: Cursor, - expectedKeys: []string{ - "cursor.general.openAIBaseURL", "http://localhost:14000/v1", - "cursor.general.openAIAPIKey", "thv-proxy", + wantPointers: map[string]string{ + "/cursor.general.openAIBaseURL": "http://localhost:14000/v1", + "/cursor.general.openAIAPIKey": "thv-proxy", }, }, { // ~/./Code/User/settings.json clientType: VSCode, - expectedKeys: []string{ - "github.copilot.advanced.serverUrl", "http://localhost:14000/v1", - "github.copilot.advanced.apiKey", "thv-proxy", + wantPointers: map[string]string{ + "/github.copilot.advanced.serverUrl": "http://localhost:14000/v1", + "/github.copilot.advanced.apiKey": "thv-proxy", }, }, { // ~/./Code - Insiders/User/settings.json clientType: VSCodeInsider, - expectedKeys: []string{ - "github.copilot.advanced.serverUrl", "http://localhost:14000/v1", - "github.copilot.advanced.apiKey", "thv-proxy", + wantPointers: map[string]string{ + "/github.copilot.advanced.serverUrl": "http://localhost:14000/v1", + "/github.copilot.advanced.apiKey": "thv-proxy", }, }, { // ~/Library/Application Support/GitHub Copilot for Xcode/editorSettings.json clientType: ClientApp(Xcode), - expectedKeys: []string{ - "openAIBaseURL", "http://localhost:14000/v1", - "apiKey", "thv-proxy", + wantPointers: map[string]string{ + "/openAIBaseURL": "http://localhost:14000/v1", + "/apiKey": "thv-proxy", }, }, } @@ -111,25 +145,26 @@ func TestRealClientConfigs_ConfigureAndRevert(t *testing.T) { settingsPath := cm.buildLLMSettingsPath(cfg) require.NoError(t, os.MkdirAll(filepath.Dir(settingsPath), 0o700)) - // Configure and verify all expected keys appear. + // Configure and verify each pointer resolves to the expected value. path, err := cm.ConfigureLLMGateway(tc.clientType, applyCfg) require.NoError(t, err) data, err := os.ReadFile(path) require.NoError(t, err) - for _, key := range tc.expectedKeys { - assert.Contains(t, string(data), key, - "key %q missing after Configure for %s", key, tc.clientType) + for ptr, want := range tc.wantPointers { + got, ok := jsonPointerGet(data, ptr) + assert.True(t, ok, "pointer %q missing after Configure for %s", ptr, tc.clientType) + assert.Equal(t, want, got, "wrong value at %q after Configure for %s", ptr, tc.clientType) } - // Revert and verify all expected keys are gone. + // Revert and verify every pointer is gone. require.NoError(t, cm.RevertLLMGateway(tc.clientType, path)) data, err = os.ReadFile(path) require.NoError(t, err) - for _, key := range tc.expectedKeys { - assert.NotContains(t, string(data), key, - "key %q still present after Revert for %s", key, tc.clientType) + for ptr := range tc.wantPointers { + _, ok := jsonPointerGet(data, ptr) + assert.False(t, ok, "pointer %q still present after Revert for %s", ptr, tc.clientType) } }) } @@ -666,35 +701,45 @@ func TestLLMValueForSpec(t *testing.T) { } cases := []struct { - name string - valueField string - cfg llmgateway.ApplyConfig - want string + name string + spec LLMGatewayKeySpec + cfg llmgateway.ApplyConfig + want string + wantErr bool }{ - {"GatewayURL", "GatewayURL", cfg, "https://gw.example.com"}, - {"ProxyBaseURL", "ProxyBaseURL", cfg, "http://localhost:14000/v1"}, - {"TokenHelperCommand", "TokenHelperCommand", cfg, `"thv" llm token`}, - {"PlaceholderAPIKey", "PlaceholderAPIKey", cfg, "thv-proxy"}, + // ValueField — known names resolve correctly + {name: "GatewayURL", spec: LLMGatewayKeySpec{ValueField: "GatewayURL"}, cfg: cfg, want: "https://gw.example.com"}, + {name: "ProxyBaseURL", spec: LLMGatewayKeySpec{ValueField: "ProxyBaseURL"}, cfg: cfg, want: "http://localhost:14000/v1"}, + {name: "TokenHelperCommand", spec: LLMGatewayKeySpec{ValueField: "TokenHelperCommand"}, cfg: cfg, want: `"thv" llm token`}, + {name: "PlaceholderAPIKey", spec: LLMGatewayKeySpec{ValueField: "PlaceholderAPIKey"}, cfg: cfg, want: "thv-proxy"}, // NodeTLSRejectUnauthorized: "0" when set, "" when clear - {"NodeTLSRejectUnauthorized/skip=false", "NodeTLSRejectUnauthorized", cfg, ""}, - {"NodeTLSRejectUnauthorized/skip=true", "NodeTLSRejectUnauthorized", llmgateway.ApplyConfig{TLSSkipVerify: true}, "0"}, + {name: "NodeTLSRejectUnauthorized/skip=false", spec: LLMGatewayKeySpec{ValueField: "NodeTLSRejectUnauthorized"}, cfg: cfg, want: ""}, + {name: "NodeTLSRejectUnauthorized/skip=true", spec: LLMGatewayKeySpec{ValueField: "NodeTLSRejectUnauthorized"}, cfg: llmgateway.ApplyConfig{TLSSkipVerify: true}, want: "0"}, // ProxyOrigin strips path, query, and fragment from ProxyBaseURL - {"ProxyOrigin/strips_path", "ProxyOrigin", cfg, "http://localhost:14000"}, - {"ProxyOrigin/long_path", "ProxyOrigin", llmgateway.ApplyConfig{ProxyBaseURL: "http://localhost:9000/v1beta/openai"}, "http://localhost:9000"}, - {"ProxyOrigin/strips_query_and_fragment", "ProxyOrigin", llmgateway.ApplyConfig{ProxyBaseURL: "http://host:8080/path?q=1#frag"}, "http://host:8080"}, + {name: "ProxyOrigin/strips_path", spec: LLMGatewayKeySpec{ValueField: "ProxyOrigin"}, cfg: cfg, want: "http://localhost:14000"}, + {name: "ProxyOrigin/long_path", spec: LLMGatewayKeySpec{ValueField: "ProxyOrigin"}, cfg: llmgateway.ApplyConfig{ProxyBaseURL: "http://localhost:9000/v1beta/openai"}, want: "http://localhost:9000"}, + {name: "ProxyOrigin/strips_query_and_fragment", spec: LLMGatewayKeySpec{ValueField: "ProxyOrigin"}, cfg: llmgateway.ApplyConfig{ProxyBaseURL: "http://host:8080/path?q=1#frag"}, want: "http://host:8080"}, // ForceQuery: trailing "?" with no key must not leak into the origin. - {"ProxyOrigin/force_query", "ProxyOrigin", llmgateway.ApplyConfig{ProxyBaseURL: "http://host:8080/path?"}, "http://host:8080"}, + {name: "ProxyOrigin/force_query", spec: LLMGatewayKeySpec{ValueField: "ProxyOrigin"}, cfg: llmgateway.ApplyConfig{ProxyBaseURL: "http://host:8080/path?"}, want: "http://host:8080"}, // ProxyOrigin falls back to the raw value when URL parsing fails - {"ProxyOrigin/invalid_url_fallback", "ProxyOrigin", llmgateway.ApplyConfig{ProxyBaseURL: "::invalid"}, "::invalid"}, - // Unrecognised ValueField is written as a literal constant - {"literal/gemini-api-key", "gemini-api-key", cfg, "gemini-api-key"}, - {"literal/some-literal", "some-literal", cfg, "some-literal"}, + {name: "ProxyOrigin/invalid_url_fallback", spec: LLMGatewayKeySpec{ValueField: "ProxyOrigin"}, cfg: llmgateway.ApplyConfig{ProxyBaseURL: "::invalid"}, want: "::invalid"}, + // Literal — written verbatim regardless of cfg + {name: "Literal/gemini-api-key", spec: LLMGatewayKeySpec{Literal: "gemini-api-key"}, cfg: cfg, want: "gemini-api-key"}, + {name: "Literal/some-constant", spec: LLMGatewayKeySpec{Literal: "some-constant"}, cfg: cfg, want: "some-constant"}, + // Unknown ValueField — must return an error (no silent literal fallback) + {name: "unknown_ValueField/typo", spec: LLMGatewayKeySpec{ValueField: "GatwayURL"}, cfg: cfg, wantErr: true}, + {name: "unknown_ValueField/arbitrary", spec: LLMGatewayKeySpec{ValueField: "not-a-field"}, cfg: cfg, wantErr: true}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { t.Parallel() - got := llmValueForSpec(tc.valueField, tc.cfg) + got, err := llmValueForSpec(tc.spec, tc.cfg) + if tc.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) assert.Equal(t, tc.want, got) }) } diff --git a/pkg/llm/setup.go b/pkg/llm/setup.go index e9e19c5d4f..c58b3d2297 100644 --- a/pkg/llm/setup.go +++ b/pkg/llm/setup.go @@ -28,6 +28,9 @@ type GatewayManager interface { ConfigureLLMGateway(clientType string, cfg llmgateway.ApplyConfig) (string, error) // LLMGatewayModeFor returns "direct", "proxy", or "" for the given client. LLMGatewayModeFor(clientType string) string + // LLMSetupNoteFor returns an optional post-setup message for the given + // client, or "" when there is nothing to surface. + LLMSetupNoteFor(clientType string) string // RevertLLMGateway removes the LLM gateway settings from the tool's config file. RevertLLMGateway(clientType, configPath string) error } @@ -343,6 +346,9 @@ func configureDetectedTools( ConfigPath: configPath, }) _, _ = fmt.Fprintf(out, "Configured %s (%s mode) → %s\n", clientType, mode, configPath) + if note := gm.LLMSetupNoteFor(clientType); note != "" { + _, _ = fmt.Fprintln(out, note) + } } if len(configured) == 0 { return nil, fmt.Errorf("failed to configure any detected tools") diff --git a/pkg/llm/setup_test.go b/pkg/llm/setup_test.go index f8d5e581a9..2bd987a5cc 100644 --- a/pkg/llm/setup_test.go +++ b/pkg/llm/setup_test.go @@ -102,6 +102,7 @@ func (*stubGatewayManager) ConfigureLLMGateway(_ string, _ llmgateway.ApplyConfi return "", nil } func (*stubGatewayManager) LLMGatewayModeFor(_ string) string { return "" } +func (*stubGatewayManager) LLMSetupNoteFor(_ string) string { return "" } func (s *stubGatewayManager) RevertLLMGateway(clientType, _ string) error { s.reverted = append(s.reverted, clientType) return nil diff --git a/pkg/llmgateway/config.go b/pkg/llmgateway/config.go index 418cf392fd..e286384123 100644 --- a/pkg/llmgateway/config.go +++ b/pkg/llmgateway/config.go @@ -7,23 +7,27 @@ package llmgateway import "net/url" -// ProxyOriginOf returns rawURL with its path, query, and fragment stripped so -// only the scheme and host remain (the "origin"). Tools like Gemini CLI that -// append their own API path (e.g. /v1beta/...) need the origin rather than the -// full proxy base URL to avoid doubled path segments. Falls back to rawURL when -// parsing fails. +// ProxyOriginOf returns rawURL with its path, query, fragment, and userinfo +// stripped so only the scheme and host remain (the "origin"). Tools like +// Gemini CLI that append their own API path (e.g. /v1beta/...) need the +// origin rather than the full proxy base URL to avoid doubled path segments. +// +// Falls back to rawURL when: +// - parsing fails, or +// - the parsed URL has no Host (e.g. scheme-less "localhost:14000/v1" is +// parsed by url.Parse as Scheme=localhost, Opaque=14000/v1 with no Host), +// or +// - the parsed URL has a non-empty Opaque field (opaque URI reference). func ProxyOriginOf(rawURL string) string { u, err := url.Parse(rawURL) - if err != nil || u == nil { + if err != nil || u == nil || u.Host == "" || u.Opaque != "" { return rawURL } - u.Path = "" - u.RawPath = "" - u.RawQuery = "" - u.ForceQuery = false - u.Fragment = "" - u.RawFragment = "" - return u.String() + origin := url.URL{ + Scheme: u.Scheme, + Host: u.Host, + } + return origin.String() } // ApplyConfig holds the values needed to configure a single tool's LLM diff --git a/pkg/llmgateway/config_test.go b/pkg/llmgateway/config_test.go new file mode 100644 index 0000000000..5a192beaa1 --- /dev/null +++ b/pkg/llmgateway/config_test.go @@ -0,0 +1,50 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package llmgateway_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/stacklok/toolhive/pkg/llmgateway" +) + +func TestProxyOriginOf(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + input string + want string + }{ + // Normal cases — path, query, and fragment stripped. + {name: "strips_path", input: "http://localhost:14000/v1", want: "http://localhost:14000"}, + {name: "strips_long_path", input: "http://localhost:9000/v1beta/openai", want: "http://localhost:9000"}, + {name: "strips_query_and_fragment", input: "http://host:8080/path?q=1#frag", want: "http://host:8080"}, + {name: "strips_fragment_only", input: "http://host:8080#frag", want: "http://host:8080"}, + // ForceQuery: trailing "?" must not persist into the origin. + {name: "strips_force_query", input: "http://host:8080/path?", want: "http://host:8080"}, + // Userinfo must not be persisted into the settings file. + {name: "strips_userinfo", input: "http://user:pass@host:8080/path", want: "http://host:8080"}, + // IPv6 host. + {name: "ipv6_host", input: "http://[::1]:14000/v1", want: "http://[::1]:14000"}, + // Empty input — url.Parse succeeds but Host is ""; fall back to rawURL. + {name: "empty_input", input: "", want: ""}, + // Scheme-less "host:port/path" — url.Parse treats this as Scheme=host, + // Opaque=port/path with no Host; fall back to rawURL. + {name: "scheme_less_host_port", input: "localhost:14000/v1", want: "localhost:14000/v1"}, + // Opaque URI — fall back to rawURL. + {name: "opaque_uri", input: "mailto:user@example.com", want: "mailto:user@example.com"}, + // Invalid URL — fall back to rawURL. + {name: "invalid_url", input: "::invalid", want: "::invalid"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, llmgateway.ProxyOriginOf(tc.input)) + }) + } +}