Skip to content

Commit 68f4c2f

Browse files
yroblataskbotclaude
authored
llm: extract shared OIDC config, move business logic to pkg/llm, add E2E tests (#5049)
* llm: extract shared OIDC config, move business logic to pkg/llm, add E2E tests Three follow-up cleanups from the #5029 review (#5048): 1. Extract shared OIDC config type into pkg/oidc.ClientConfig. Both config.RegistryOAuthConfig and llm.OIDCConfig are now type aliases for this type, so field additions and validation changes apply to both authentication flows without drift. 2. Move business logic from the CLI layer into pkg/llm per the cli-commands convention. The config-set mutation/validation branch, the config-show text formatter, and the cached-token deletion logic all live in pkg/llm now. The CLI commands are thin wrappers that parse flags and delegate. 3. Add E2E tests for thv llm config set/show/reset, covering happy-path round-trips, HTTPS enforcement, incremental configuration, the not-configured message, and post-reset state. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * llm: fix errcheck lint failures in Show method Show now returns an error and uses a writef closure to propagate the first write failure rather than ignoring io.Writer errors. The CLI caller is updated to return the error directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * llm: skip token deletion for providers that can't list or delete Providers like the environment provider are read-only and cannot hold cached tokens, so calling ListSecrets on them would always produce a spurious warning during thv llm config reset. Check CanList/CanDelete capabilities first and treat non-capable providers as a no-op. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * llm: add unit tests for SetFields, DeleteCachedTokens, and Show Covers the business logic introduced in manage.go: - SetFields: field mutation, partial vs full validation branching, invalid-value rejection - DeleteCachedTokens: capability no-ops (no CanList/CanDelete), empty-scope no-op, scoped deletion, error propagation - Show: not-configured message, required-field output, conditional audience line, configured-tools section Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(e2e): configure environment secrets provider in llm config tests Mirrors the pattern from cli_secrets_scoped_test.go to ensure the test suite never opens the user's real keychain or 1Password. The environment provider is non-interactive and read-only, so DeleteCachedTokens is a no-op, keeping the tests fully hermetic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: taskbot <taskbot@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 854e0ab commit 68f4c2f

8 files changed

Lines changed: 636 additions & 128 deletions

File tree

‎cmd/thv/app/llm.go‎

Lines changed: 13 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -69,14 +69,7 @@ func newConfigCommand() *cobra.Command {
6969
}
7070

7171
func newConfigSetCommand() *cobra.Command {
72-
var (
73-
gatewayURL string
74-
issuer string
75-
clientID string
76-
audience string
77-
proxyPort int
78-
callbackPort int
79-
)
72+
var opts llm.SetOptions
8073

8174
cmd := &cobra.Command{
8275
Use: "set",
@@ -91,43 +84,17 @@ Example:
9184
Args: cobra.NoArgs,
9285
RunE: func(_ *cobra.Command, _ []string) error {
9386
return config.UpdateConfig(func(c *config.Config) error {
94-
if gatewayURL != "" {
95-
c.LLM.GatewayURL = gatewayURL
96-
}
97-
if issuer != "" {
98-
c.LLM.OIDC.Issuer = issuer
99-
}
100-
if clientID != "" {
101-
c.LLM.OIDC.ClientID = clientID
102-
}
103-
if audience != "" {
104-
c.LLM.OIDC.Audience = audience
105-
}
106-
if proxyPort != 0 {
107-
c.LLM.Proxy.ListenPort = proxyPort
108-
}
109-
if callbackPort != 0 {
110-
c.LLM.OIDC.CallbackPort = callbackPort
111-
}
112-
// Always validate format/range for any fields that were set,
113-
// so invalid values (e.g. http:// URL, out-of-range port) are
114-
// rejected immediately rather than silently persisted.
115-
// Full validation (required-field checks) only runs once the
116-
// mandatory trio is present, allowing incremental configuration.
117-
if !c.LLM.IsConfigured() {
118-
return c.LLM.ValidatePartial()
119-
}
120-
return c.LLM.Validate()
87+
return c.LLM.SetFields(opts)
12188
})
12289
},
12390
}
12491

125-
cmd.Flags().StringVar(&gatewayURL, "gateway-url", "", "LLM gateway base URL (must use HTTPS)")
126-
cmd.Flags().StringVar(&issuer, "issuer", "", "OIDC issuer URL")
127-
cmd.Flags().StringVar(&clientID, "client-id", "", "OIDC client ID")
128-
cmd.Flags().StringVar(&audience, "audience", "", "OIDC audience (optional)")
129-
cmd.Flags().IntVar(&proxyPort, "proxy-port", 0, "Localhost proxy listen port (default 14000)")
130-
cmd.Flags().IntVar(&callbackPort, "callback-port", 0, "OIDC callback port (default: ephemeral)")
92+
cmd.Flags().StringVar(&opts.GatewayURL, "gateway-url", "", "LLM gateway base URL (must use HTTPS)")
93+
cmd.Flags().StringVar(&opts.Issuer, "issuer", "", "OIDC issuer URL")
94+
cmd.Flags().StringVar(&opts.ClientID, "client-id", "", "OIDC client ID")
95+
cmd.Flags().StringVar(&opts.Audience, "audience", "", "OIDC audience (optional)")
96+
cmd.Flags().IntVar(&opts.ProxyPort, "proxy-port", 0, "Localhost proxy listen port (default 14000)")
97+
cmd.Flags().IntVar(&opts.CallbackPort, "callback-port", 0, "OIDC callback port (default: ephemeral)")
13198

13299
return cmd
133100
}
@@ -153,27 +120,7 @@ func newConfigShowCommand() *cobra.Command {
153120
return nil
154121
}
155122

156-
if !llmCfg.IsConfigured() {
157-
fmt.Println("LLM gateway is not configured. Run \"thv llm config set\" to configure it.")
158-
return nil
159-
}
160-
161-
fmt.Printf("Gateway URL: %s\n", llmCfg.GatewayURL)
162-
fmt.Printf("OIDC Issuer: %s\n", llmCfg.OIDC.Issuer)
163-
fmt.Printf("OIDC Client: %s\n", llmCfg.OIDC.ClientID)
164-
if llmCfg.OIDC.Audience != "" {
165-
fmt.Printf("Audience: %s\n", llmCfg.OIDC.Audience)
166-
}
167-
fmt.Printf("Proxy Port: %d\n", llmCfg.EffectiveProxyPort())
168-
fmt.Printf("Scopes: %v\n", llmCfg.OIDC.EffectiveScopes())
169-
if len(llmCfg.ConfiguredTools) > 0 {
170-
fmt.Println("Configured tools:")
171-
for _, t := range llmCfg.ConfiguredTools {
172-
fmt.Printf(" - %s (%s) %s\n", t.Tool, t.Mode, t.ConfigPath)
173-
}
174-
}
175-
176-
return nil
123+
return llmCfg.Show(os.Stdout)
177124
},
178125
}
179126

@@ -191,8 +138,11 @@ tokens from the secrets provider.`,
191138
Args: cobra.NoArgs,
192139
RunE: func(cmd *cobra.Command, _ []string) error {
193140
// Delete cached tokens from the secrets provider first.
194-
if err := deleteLLMSecrets(cmd.Context()); err != nil {
141+
provider, err := secrets.GetSystemSecretsProvider()
142+
if err != nil {
195143
// Non-fatal: log and continue so the config is still cleared.
144+
fmt.Fprintf(os.Stderr, "Warning: could not get secrets provider: %v\n", err)
145+
} else if err := llm.DeleteCachedTokens(cmd.Context(), provider); err != nil {
196146
fmt.Fprintf(os.Stderr, "Warning: could not remove cached LLM tokens: %v\n", err)
197147
}
198148

@@ -241,27 +191,6 @@ func runLLMToken(ctx context.Context) error {
241191
return nil
242192
}
243193

244-
// deleteLLMSecrets removes all secrets stored under the LLM scope.
245-
func deleteLLMSecrets(ctx context.Context) error {
246-
provider, err := secrets.GetSystemSecretsProvider()
247-
if err != nil {
248-
return fmt.Errorf("failed to get secrets provider: %w", err)
249-
}
250-
scoped := pkgsecrets.NewScopedProvider(provider, pkgsecrets.ScopeLLM)
251-
descs, err := scoped.ListSecrets(ctx)
252-
if err != nil {
253-
return err
254-
}
255-
if len(descs) == 0 {
256-
return nil
257-
}
258-
names := make([]string, len(descs))
259-
for i, d := range descs {
260-
names[i] = d.Key
261-
}
262-
return scoped.DeleteSecrets(ctx, names)
263-
}
264-
265194
// ── setup / teardown stubs ────────────────────────────────────────────────────
266195

267196
func newLLMSetupCommand() *cobra.Command {

‎pkg/config/config.go‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"github.com/stacklok/toolhive/pkg/container/templates"
2222
"github.com/stacklok/toolhive/pkg/llm"
2323
"github.com/stacklok/toolhive/pkg/lockfile"
24+
"github.com/stacklok/toolhive/pkg/oidc"
2425
"github.com/stacklok/toolhive/pkg/secrets"
2526
)
2627

@@ -65,17 +66,10 @@ type RegistryAuth struct {
6566

6667
// RegistryOAuthConfig holds OAuth/OIDC configuration for registry authentication.
6768
// PKCE (S256) is always enforced per OAuth 2.1 requirements for public clients.
68-
type RegistryOAuthConfig struct {
69-
Issuer string `yaml:"issuer"`
70-
ClientID string `yaml:"client_id"`
71-
Scopes []string `yaml:"scopes,omitempty"`
72-
Audience string `yaml:"audience,omitempty"`
73-
CallbackPort int `yaml:"callback_port,omitempty"`
74-
75-
// Cached token references for session restoration across CLI invocations.
76-
CachedRefreshTokenRef string `yaml:"cached_refresh_token_ref,omitempty"`
77-
CachedTokenExpiry time.Time `yaml:"cached_token_expiry,omitempty"`
78-
}
69+
//
70+
// This is a type alias for oidc.ClientConfig so that registry and LLM gateway
71+
// authentication always share the same field set and validation logic.
72+
type RegistryOAuthConfig = oidc.ClientConfig
7973

8074
// Secrets contains the settings for secrets management.
8175
type Secrets struct {

‎pkg/llm/config.go‎

Lines changed: 7 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -6,20 +6,22 @@ package llm
66
import (
77
"errors"
88
"fmt"
9-
"strings"
10-
"time"
119

1210
"github.com/stacklok/toolhive/pkg/networking"
11+
pkgoidc "github.com/stacklok/toolhive/pkg/oidc"
1312
)
1413

1514
const (
1615
// DefaultProxyListenPort is the default port the localhost proxy listens on.
1716
DefaultProxyListenPort = 14000
18-
19-
// DefaultOIDCScopes are the default OAuth scopes requested during login.
20-
DefaultOIDCScopes = "openid offline_access"
2117
)
2218

19+
// OIDCConfig is a type alias for oidc.ClientConfig, holding OIDC provider
20+
// settings and cached token state for the LLM gateway. Using a type alias
21+
// ensures this type stays in sync with pkg/config.RegistryOAuthConfig, which
22+
// is also an alias for the same underlying type.
23+
type OIDCConfig = pkgoidc.ClientConfig
24+
2325
// Config holds all LLM gateway settings persisted under the llm: key in
2426
// ToolHive's config.yaml.
2527
type Config struct {
@@ -29,25 +31,6 @@ type Config struct {
2931
ConfiguredTools []ToolConfig `yaml:"configured_tools,omitempty" json:"configured_tools,omitempty"`
3032
}
3133

32-
// OIDCConfig holds OIDC provider settings and cached token state for the LLM
33-
// gateway. Cached fields follow the same pattern as RegistryOAuthConfig in
34-
// pkg/config/config.go — token values are never stored here, only references
35-
// and expiry metadata.
36-
type OIDCConfig struct {
37-
Issuer string `yaml:"issuer,omitempty" json:"issuer,omitempty"`
38-
ClientID string `yaml:"client_id,omitempty" json:"client_id,omitempty"`
39-
Scopes []string `yaml:"scopes,omitempty" json:"scopes,omitempty"`
40-
Audience string `yaml:"audience,omitempty" json:"audience,omitempty"`
41-
CallbackPort int `yaml:"callback_port,omitempty" json:"callback_port,omitempty"`
42-
43-
// CachedRefreshTokenRef is the secrets-provider key under which the refresh
44-
// token is stored (never the token value itself).
45-
CachedRefreshTokenRef string `yaml:"cached_refresh_token_ref,omitempty" json:"cached_refresh_token_ref,omitempty"`
46-
// CachedTokenExpiry is the expiry of the most recently cached access token,
47-
// used to surface helpful messages when the token is about to expire.
48-
CachedTokenExpiry time.Time `yaml:"cached_token_expiry,omitempty" json:"cached_token_expiry,omitempty"`
49-
}
50-
5134
// ProxyConfig holds configuration for the localhost reverse proxy.
5235
type ProxyConfig struct {
5336
ListenPort int `yaml:"listen_port,omitempty" json:"listen_port,omitempty"`
@@ -132,12 +115,3 @@ func (c *Config) EffectiveProxyPort() int {
132115
}
133116
return DefaultProxyListenPort
134117
}
135-
136-
// EffectiveScopes returns the configured OIDC scopes, or the default scopes
137-
// (openid, offline_access) if none are set.
138-
func (c *OIDCConfig) EffectiveScopes() []string {
139-
if len(c.Scopes) > 0 {
140-
return c.Scopes
141-
}
142-
return strings.Fields(DefaultOIDCScopes)
143-
}

‎pkg/llm/manage.go‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package llm
5+
6+
import (
7+
"context"
8+
"fmt"
9+
"io"
10+
11+
pkgsecrets "github.com/stacklok/toolhive/pkg/secrets"
12+
)
13+
14+
// SetFields applies the non-zero fields from the provided options to the
15+
// config and validates the result. If the mandatory trio (gateway_url,
16+
// oidc.issuer, oidc.client_id) is present after the update, full validation
17+
// runs; otherwise only format/range validation runs to catch bad values early
18+
// while still allowing incremental configuration.
19+
func (c *Config) SetFields(opts SetOptions) error {
20+
if opts.GatewayURL != "" {
21+
c.GatewayURL = opts.GatewayURL
22+
}
23+
if opts.Issuer != "" {
24+
c.OIDC.Issuer = opts.Issuer
25+
}
26+
if opts.ClientID != "" {
27+
c.OIDC.ClientID = opts.ClientID
28+
}
29+
if opts.Audience != "" {
30+
c.OIDC.Audience = opts.Audience
31+
}
32+
if opts.ProxyPort != 0 {
33+
c.Proxy.ListenPort = opts.ProxyPort
34+
}
35+
if opts.CallbackPort != 0 {
36+
c.OIDC.CallbackPort = opts.CallbackPort
37+
}
38+
39+
if !c.IsConfigured() {
40+
return c.ValidatePartial()
41+
}
42+
return c.Validate()
43+
}
44+
45+
// SetOptions carries the flag values for the "config set" command.
46+
// Zero values are treated as "not provided" and leave the existing config
47+
// field unchanged.
48+
type SetOptions struct {
49+
GatewayURL string
50+
Issuer string
51+
ClientID string
52+
Audience string
53+
ProxyPort int
54+
CallbackPort int
55+
}
56+
57+
// DeleteCachedTokens removes all cached OIDC tokens stored under the LLM
58+
// scope via the provided secrets provider. It is a no-op if the provider does
59+
// not support listing or deletion (e.g. the environment provider), since such
60+
// providers cannot hold cached tokens.
61+
func DeleteCachedTokens(ctx context.Context, provider pkgsecrets.Provider) error {
62+
scoped := pkgsecrets.NewScopedProvider(provider, pkgsecrets.ScopeLLM)
63+
caps := scoped.Capabilities()
64+
if !caps.CanList || !caps.CanDelete {
65+
return nil
66+
}
67+
descs, err := scoped.ListSecrets(ctx)
68+
if err != nil {
69+
return err
70+
}
71+
if len(descs) == 0 {
72+
return nil
73+
}
74+
names := make([]string, len(descs))
75+
for i, d := range descs {
76+
names[i] = d.Key
77+
}
78+
return scoped.DeleteSecrets(ctx, names)
79+
}
80+
81+
// Show writes a human-readable representation of the config to w.
82+
// If the config is not yet configured it prints a hint to run "config set".
83+
func (c *Config) Show(w io.Writer) error {
84+
if !c.IsConfigured() {
85+
_, err := fmt.Fprintln(w, "LLM gateway is not configured. Run \"thv llm config set\" to configure it.")
86+
return err
87+
}
88+
89+
var err error
90+
writef := func(format string, args ...any) {
91+
if err == nil {
92+
_, err = fmt.Fprintf(w, format, args...)
93+
}
94+
}
95+
96+
writef("Gateway URL: %s\n", c.GatewayURL)
97+
writef("OIDC Issuer: %s\n", c.OIDC.Issuer)
98+
writef("OIDC Client: %s\n", c.OIDC.ClientID)
99+
if c.OIDC.Audience != "" {
100+
writef("Audience: %s\n", c.OIDC.Audience)
101+
}
102+
writef("Proxy Port: %d\n", c.EffectiveProxyPort())
103+
writef("Scopes: %v\n", c.OIDC.EffectiveScopes())
104+
if len(c.ConfiguredTools) > 0 {
105+
writef("Configured tools:\n")
106+
for _, t := range c.ConfiguredTools {
107+
writef(" - %s (%s) %s\n", t.Tool, t.Mode, t.ConfigPath)
108+
}
109+
}
110+
return err
111+
}

0 commit comments

Comments
 (0)