Detailed changes
@@ -298,12 +298,31 @@ types: `stdio` for command-line servers, `http` for HTTP endpoints, and `sse`
for Server-Sent Events.
Shell-style value expansion (`$VAR`, `${VAR:-default}`, `$(command)`, quoting,
-and nesting (works in `command`, `args`, `env`, `headers`, and `url`, so
-file-based secrets like work out of the box, so you can use values like
-"$TOKEN"` and `"$(cat /path/to/secret/token)"``. Expansion runs through Crush's
-embedded shell, so the same syntax works on all supported systems, including
-Windows. Unset variables are an error; use `${VAR:-fallback}` to opt in to
-a default.
+nesting) works in `command`, `args`, `env`, `headers`, and `url`, so
+file-based secrets work out of the box. You can use values like `"$TOKEN"`
+or `"$(cat /path/to/secret/token)"`. Expansion runs through Crush's embedded
+shell, so the same syntax works on every supported system, Windows included.
+
+Unset variables expand to the empty string by default, matching bash. For
+required credentials, use `${VAR:?message}` so an unset variable fails loudly
+at load time with `message` instead of silently resolving to empty:
+
+```json
+{ "api_key": "${CODEBERG_TOKEN:?set CODEBERG_TOKEN}" }
+```
+
+Headers (both MCP `headers` and provider `extra_headers`) whose value
+resolves to the empty string are dropped from the outgoing request rather
+than sent as `Header:`. That keeps optional env-gated headers like
+`"OpenAI-Organization": "$OPENAI_ORG_ID"` clean when the variable is unset.
+
+Provider `extra_body` is a non-expanding JSON passthrough; put env-driven
+values in `extra_headers` or the provider's `api_key` / `base_url`, all of
+which do expand.
+
+> **Security note:** `crush.json` is trusted code. Any `$(...)` in it runs at
+> load time with your shell's privileges, before the UI appears. Don't launch
+> Crush in a directory whose `crush.json` you haven't reviewed.
```json
{
@@ -91,17 +91,40 @@ func TestCreateTransport_URLResolution(t *testing.T) {
require.Equal(t, "https://mcp.example.com/events", sse.Endpoint)
})
- t.Run("http unset var surfaces error, no transport created", func(t *testing.T) {
+ t.Run("http failing $(cmd) surfaces error, no transport created", func(t *testing.T) {
t.Parallel()
+ // Under lenient nounset, unset $VAR expands to "" silently,
+ // so the only way a URL resolution *errors* is a failing
+ // $(cmd). Mirror the SSE subtest so both transports share
+ // coverage for the url-resolve-failure path.
m := config.MCPConfig{
Type: config.MCPHttp,
- URL: "https://$MCP_MISSING_HOST/api",
+ URL: "https://$(false)/api",
}
- tr, err := createTransport(t.Context(), m, shell)
+ tr, err := createTransport(t.Context(), m, shellResolverWithPath(t, nil))
require.Error(t, err)
require.Nil(t, tr)
require.Contains(t, err.Error(), "url:")
- require.Contains(t, err.Error(), "$MCP_MISSING_HOST")
+ require.Contains(t, err.Error(), "$(false)")
+ })
+
+ t.Run("http unset var expands empty", func(t *testing.T) {
+ t.Parallel()
+ // Pinning test for the new lenient-nounset default: an
+ // unset bare $VAR in the URL is *not* an error. It
+ // expands to "" and, here, leaves a syntactically weird
+ // but non-empty URL that the existing non-empty guard
+ // still lets through. Guards against a future regression
+ // that flips strict-by-default back on.
+ m := config.MCPConfig{
+ Type: config.MCPHttp,
+ URL: "https://$MCP_MISSING_HOST/api",
+ }
+ tr, err := createTransport(t.Context(), m, shell)
+ require.NoError(t, err)
+ sct, ok := tr.(*mcp.StreamableClientTransport)
+ require.True(t, ok)
+ require.Equal(t, "https:///api", sct.Endpoint)
})
t.Run("sse failing $(cmd) surfaces error, no transport created", func(t *testing.T) {
@@ -201,16 +224,18 @@ func TestCreateTransport_StdioResolution(t *testing.T) {
require.Contains(t, err.Error(), "env TOKEN")
})
- t.Run("unset env var is a hard error, not silent empty", func(t *testing.T) {
+ t.Run("failing env command is a hard error", func(t *testing.T) {
t.Parallel()
- // The regression at the heart of PLAN.md: unset vars used to
- // silently substitute "" and hand an empty credential to the
- // child process. Now they must error out before exec.
+ // Under lenient nounset a bare $UNSET expands to ""
+ // silently — see the pinning subtest below. The remaining
+ // failure mode for env resolution is a $(cmd) that exits
+ // non-zero, which must still error out and prevent exec so
+ // we never hand a broken credential to the child process.
r := shellResolverWithPath(t, nil)
m := config.MCPConfig{
Type: config.MCPStdio,
Command: "forgejo-mcp",
- Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$FORGJO_TOKEN"},
+ Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$(exit 5)"},
}
tr, err := createTransport(t.Context(), m, r)
require.Error(t, err)
@@ -218,6 +243,28 @@ func TestCreateTransport_StdioResolution(t *testing.T) {
require.Contains(t, err.Error(), "env FORGEJO_ACCESS_TOKEN")
})
+ t.Run("unset env var expands empty", func(t *testing.T) {
+ t.Parallel()
+ // Pinning test for the lenient-nounset default: a bare
+ // $UNSET in an env value expands to "" without error, and
+ // the empty entry is kept on the resulting exec.Cmd (env
+ // entries, unlike headers, are not dropped — see design
+ // decision #18). Guards against a regression that flips
+ // strict-by-default back on and silently breaks users
+ // with configs like FORGEJO_ACCESS_TOKEN=$FORGEJO_TOKEN.
+ r := shellResolverWithPath(t, nil)
+ m := config.MCPConfig{
+ Type: config.MCPStdio,
+ Command: "forgejo-mcp",
+ Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$FORGEJO_TOKEN_UNSET"},
+ }
+ tr, err := createTransport(t.Context(), m, r)
+ require.NoError(t, err)
+ ct, ok := tr.(*mcp.CommandTransport)
+ require.True(t, ok)
+ require.Contains(t, ct.Command.Env, "FORGEJO_ACCESS_TOKEN=")
+ })
+
t.Run("args resolution failure surfaces error, no transport created", func(t *testing.T) {
t.Parallel()
r := shellResolverWithPath(t, nil)
@@ -311,17 +358,43 @@ func TestCreateTransport_HeadersResolution(t *testing.T) {
t.Run("sse failing header surfaces error, no transport", func(t *testing.T) {
t.Parallel()
+ // Under lenient nounset a bare $MISSING expands to "",
+ // which ResolvedHeaders drops — no error. The failing
+ // $(cmd) path is the remaining way this can fail loudly;
+ // cover it on the SSE branch to mirror the HTTP subtest.
r := shellResolverWithPath(t, nil)
m := config.MCPConfig{
Type: config.MCPSSE,
URL: "https://mcp.example.com/events",
- Headers: map[string]string{"Authorization": "Bearer $MISSING_TOKEN"},
+ Headers: map[string]string{"Authorization": "$(false)"},
}
tr, err := createTransport(t.Context(), m, r)
require.Error(t, err)
require.Nil(t, tr)
require.Contains(t, err.Error(), "header Authorization")
- require.Contains(t, err.Error(), "$MISSING_TOKEN")
+ })
+
+ t.Run("sse unset var header drops silently", func(t *testing.T) {
+ t.Parallel()
+ // Pinning test for design decision #18 + lenient nounset:
+ // a header whose value resolves to "" (here because the
+ // bare $VAR is unset) is omitted from the round tripper
+ // rather than sent as "X-Header:". Guards against a
+ // regression that either re-introduces strict-by-default
+ // or stops dropping empty headers.
+ r := shellResolverWithPath(t, nil)
+ m := config.MCPConfig{
+ Type: config.MCPSSE,
+ URL: "https://mcp.example.com/events",
+ Headers: map[string]string{"Authorization": "$MISSING_TOKEN"},
+ }
+ tr, err := createTransport(t.Context(), m, r)
+ require.NoError(t, err)
+ sse, ok := tr.(*mcp.SSEClientTransport)
+ require.True(t, ok)
+ rt, ok := sse.HTTPClient.Transport.(*headerRoundTripper)
+ require.True(t, ok)
+ require.NotContains(t, rt.headers, "Authorization")
})
}
@@ -356,24 +429,44 @@ func TestCreateSession_ResolutionFailureUpdatesState(t *testing.T) {
wantErrContains: "env FORGEJO_ACCESS_TOKEN",
},
{
+ // Args that reference an unset bare $VAR no longer
+ // error out under lenient nounset; the only remaining
+ // failure mode for arg resolution is a failing $(cmd).
name: "stdio args failure",
mcpName: "test-stdio-args-fail",
cfg: config.MCPConfig{
Type: config.MCPStdio,
Command: "echo",
- Args: []string{"--token", "$MCP_UNSET_TOKEN"},
+ Args: []string{"--token", "$(false)"},
},
wantErrContains: "arg 1",
},
{
+ // Likewise for URL: bare $UNSET expands to ""
+ // silently, so we need a failing $(cmd) to exercise
+ // the "url:" wrap from ResolvedURL.
name: "http url failure",
mcpName: "test-http-url-fail",
cfg: config.MCPConfig{
Type: config.MCPHttp,
- URL: "https://$MCP_MISSING_HOST/api",
+ URL: "https://$(false)/api",
},
wantErrContains: "url:",
},
+ {
+ // A URL whose shell expansion yields the empty
+ // string (here via ${VAR:-}) is not a ResolvedURL
+ // error, but the non-empty guard in createTransport
+ // must still reject it so the state card renders an
+ // error instead of spawning a transport against "".
+ name: "http empty-resolved url",
+ mcpName: "test-http-url-empty",
+ cfg: config.MCPConfig{
+ Type: config.MCPHttp,
+ URL: "${MCP_URL_EMPTY:-}",
+ },
+ wantErrContains: "non-empty 'url'",
+ },
{
name: "http header failure",
mcpName: "test-http-header-fail",
@@ -394,12 +487,17 @@ func TestCreateSession_ResolutionFailureUpdatesState(t *testing.T) {
wantErrContains: "url:",
},
{
+ // Bare $MISSING in a header resolves to "" silently
+ // and is then dropped (design decision #18). The
+ // "header Authorization" wrap only surfaces on a
+ // $(cmd) failure; that is what this subtest now
+ // pins for the SSE path.
name: "sse header failure",
mcpName: "test-sse-header-fail",
cfg: config.MCPConfig{
Type: config.MCPSSE,
URL: "https://mcp.example.com/events",
- Headers: map[string]string{"Authorization": "Bearer $MISSING_SSE_TOKEN"},
+ Headers: map[string]string{"Authorization": "$(false)"},
},
wantErrContains: "header Authorization",
},
@@ -108,9 +108,22 @@ type ProviderConfig struct {
// Custom system prompt prefix.
SystemPromptPrefix string `json:"system_prompt_prefix,omitempty" jsonschema:"description=Custom prefix to add to system prompts for this provider"`
- // Extra headers to send with each request to the provider.
+ // Extra headers to send with each request to the provider. Values
+ // run through shell expansion at config-load time, so $VAR and
+ // $(cmd) work the same way they do in MCP headers. A header whose
+ // value resolves to the empty string (unset bare $VAR under
+ // lenient nounset, $(echo), or literal "") is omitted from the
+ // outgoing request rather than sent as "Header:". See PLAN.md
+ // Phase 2 design decision #18.
ExtraHeaders map[string]string `json:"extra_headers,omitempty" jsonschema:"description=Additional HTTP headers to send with requests"`
- // Extra body
+ // ExtraBody is merged verbatim into OpenAI-compatible request
+ // bodies. String values are NOT shell-expanded: this is a plain
+ // JSON passthrough so that arbitrary provider-extension fields
+ // (numbers, nested objects, booleans) round-trip without a
+ // recursive walker guessing at intent. If you need an env-var-
+ // driven value at request time, put it in extra_headers, or in
+ // the provider's top-level api_key / base_url, all of which do
+ // expand. See PLAN.md Phase 2 design decision #16.
ExtraBody map[string]any `json:"extra_body,omitempty" jsonschema:"description=Additional fields to include in request bodies, only works with openai-compatible providers"`
ProviderOptions map[string]any `json:"provider_options,omitempty" jsonschema:"description=Additional provider-specific options for this provider"`
@@ -177,7 +190,12 @@ type MCPConfig struct {
DisabledTools []string `json:"disabled_tools,omitempty" jsonschema:"description=List of tools from this MCP server to disable,example=get-library-doc"`
Timeout int `json:"timeout,omitempty" jsonschema:"description=Timeout in seconds for MCP server connections,default=15,example=30,example=60,example=120"`
- // TODO: maybe make it possible to get the value from the env
+ // Headers are HTTP headers for HTTP/SSE MCP servers. Values run
+ // through shell expansion at MCP startup, so $VAR and $(cmd)
+ // work. A header whose value resolves to the empty string (unset
+ // bare $VAR under lenient nounset, $(echo), or literal "") is
+ // omitted from the outgoing request rather than sent as
+ // "Header:". See PLAN.md Phase 2 design decision #18.
Headers map[string]string `json:"headers,omitempty" jsonschema:"description=HTTP headers for HTTP/SSE MCP servers"`
}
@@ -371,6 +389,13 @@ func (m MCPConfig) ResolvedURL(r VariableResolver) (string, error) {
// already sanitized by ResolveValue and is wrapped with %w so
// errors.Is/As continues to work.
//
+// A header whose value resolves to the empty string (unset bare $VAR
+// under lenient nounset, $(echo), or literal "") is omitted from the
+// returned map — sending "X-Auth:" with an empty value is rejected by
+// some providers and the user's intent in "optional, env-gated
+// header" is clearly "absent when the var isn't set." See PLAN.md
+// Phase 2 design decision #18.
+//
// See ResolvedEnv for guidance on picking a resolver.
func (m MCPConfig) ResolvedHeaders(r VariableResolver) (map[string]string, error) {
if len(m.Headers) == 0 {
@@ -389,6 +414,80 @@ func (m MCPConfig) ResolvedHeaders(r VariableResolver) (map[string]string, error
if err != nil {
return nil, fmt.Errorf("header %s: %w", k, err)
}
+ if v == "" {
+ continue
+ }
+ out[k] = v
+ }
+ return out, nil
+}
+
+// ResolvedArgs returns l.Args with every element expanded through the
+// given resolver. A fresh slice is allocated; l.Args is never mutated.
+// On the first resolution failure it returns nil and an error
+// identifying the offending positional index; the inner resolver error
+// is already sanitized by ResolveValue and is wrapped with %w so
+// errors.Is/As continues to work.
+//
+// Empty resolved values are kept (a deliberate "empty positional arg"
+// like --flag "" is sometimes valid), matching MCPConfig.ResolvedArgs;
+// see PLAN.md Phase 2 design decision #18.
+//
+// The resolver choice matters: in server mode pass the shell resolver
+// so $VAR / $(cmd) expand; in client mode pass IdentityResolver so the
+// template is forwarded verbatim. See PLAN.md Phase 2 design decision
+// #13.
+func (l LSPConfig) ResolvedArgs(r VariableResolver) ([]string, error) {
+ if len(l.Args) == 0 {
+ return nil, nil
+ }
+ out := make([]string, len(l.Args))
+ for i, a := range l.Args {
+ v, err := r.ResolveValue(a)
+ if err != nil {
+ return nil, fmt.Errorf("arg %d: %w", i, err)
+ }
+ out[i] = v
+ }
+ return out, nil
+}
+
+// ResolvedEnv returns l.Env with every value expanded through the
+// given resolver. A fresh map is allocated; l.Env is never mutated.
+// On the first resolution failure it returns nil and an error that
+// identifies the offending key; the inner resolver error is already
+// sanitized by ResolveValue and is wrapped with %w so errors.Is/As
+// continues to work.
+//
+// Empty resolved values are kept ("FOO=" is a legitimate request;
+// opt out via ${VAR:+...}), matching MCPConfig.ResolvedEnv; see
+// PLAN.md Phase 2 design decision #18.
+//
+// Shape note: this returns map[string]string rather than the []string
+// shape MCPConfig.ResolvedEnv uses because the consumer
+// (powernap.ClientConfig.Environment in internal/lsp/client.go) takes
+// a map directly — returning a []string here would only force a
+// round-trip back to a map at the call site. See PLAN.md Phase 2
+// design decision #13.
+//
+// See ResolvedArgs for guidance on picking a resolver.
+func (l LSPConfig) ResolvedEnv(r VariableResolver) (map[string]string, error) {
+ if len(l.Env) == 0 {
+ return map[string]string{}, nil
+ }
+ out := make(map[string]string, len(l.Env))
+ // Sort keys so failures are reported deterministically when more
+ // than one value would fail.
+ keys := make([]string, 0, len(l.Env))
+ for k := range l.Env {
+ keys = append(keys, k)
+ }
+ slices.Sort(keys)
+ for _, k := range keys {
+ v, err := r.ResolveValue(l.Env[k])
+ if err != nil {
+ return nil, fmt.Errorf("env %q: %w", k, err)
+ }
out[k] = v
}
return out, nil
@@ -222,16 +222,19 @@ func (c *Config) configureProviders(store *ConfigStore, env env.Env, resolver Va
if len(config.ExtraHeaders) > 0 {
maps.Copy(headers, config.ExtraHeaders)
}
- // Intentional divergence from MCP env/headers/args/url resolution:
- // provider headers that fail to resolve log and keep their literal
- // template so providers with optional, env-gated headers still
- // load on hosts where those vars are unset. Changing this to a
- // hard error would break those configs. See PLAN.md "Design
- // decisions" item 4 for the full rationale.
+ // Provider headers use the same error contract as MCP headers:
+ // a failing $(...) aborts the provider load with a clear
+ // message, and a header that resolves to the empty string
+ // (unset bare $VAR under lenient nounset, $(echo), or literal
+ // "") is dropped from the outgoing request. See PLAN.md
+ // Phase 2 design decisions #14 and #18.
for k, v := range headers {
resolved, err := resolver.ResolveValue(v)
if err != nil {
- slog.Error("Could not resolve provider header", "err", err.Error())
+ return fmt.Errorf("resolving provider %s header %q: %w", p.ID, k, err)
+ }
+ if resolved == "" {
+ delete(headers, k)
continue
}
headers[k] = resolved
@@ -382,10 +385,16 @@ func (c *Config) configureProviders(store *ConfigStore, env env.Env, resolver Va
continue
}
+ // Custom-provider headers share the MCP error contract; see
+ // the known-provider loop above and PLAN.md Phase 2 design
+ // decisions #14 and #18.
for k, v := range providerConfig.ExtraHeaders {
resolved, err := resolver.ResolveValue(v)
if err != nil {
- slog.Error("Could not resolve provider header", "err", err.Error())
+ return fmt.Errorf("resolving provider %s header %q: %w", id, k, err)
+ }
+ if resolved == "" {
+ delete(providerConfig.ExtraHeaders, k)
continue
}
providerConfig.ExtraHeaders[k] = resolved
@@ -77,7 +77,7 @@ func TestConfig_configureProviders(t *testing.T) {
env := env.NewFromMap(map[string]string{
"OPENAI_API_KEY": "test-key",
})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
require.Equal(t, 1, cfg.Providers.Len())
@@ -120,7 +120,7 @@ func TestConfig_configureProvidersWithOverride(t *testing.T) {
env := env.NewFromMap(map[string]string{
"OPENAI_API_KEY": "test-key",
})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
require.Equal(t, 1, cfg.Providers.Len())
@@ -162,7 +162,7 @@ func TestConfig_configureProvidersWithNewProvider(t *testing.T) {
env := env.NewFromMap(map[string]string{
"OPENAI_API_KEY": "test-key",
})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
// Should be to because of the env variable
@@ -198,7 +198,7 @@ func TestConfig_configureProvidersBedrockWithCredentials(t *testing.T) {
"AWS_ACCESS_KEY_ID": "test-key-id",
"AWS_SECRET_ACCESS_KEY": "test-secret-key",
})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
require.Equal(t, cfg.Providers.Len(), 1)
@@ -224,7 +224,7 @@ func TestConfig_configureProvidersBedrockWithoutCredentials(t *testing.T) {
cfg := &Config{}
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
// Provider should not be configured without credentials
@@ -249,7 +249,7 @@ func TestConfig_configureProvidersBedrockWithoutUnsupportedModel(t *testing.T) {
"AWS_ACCESS_KEY_ID": "test-key-id",
"AWS_SECRET_ACCESS_KEY": "test-secret-key",
})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.Error(t, err)
}
@@ -272,7 +272,7 @@ func TestConfig_configureProvidersVertexAIWithCredentials(t *testing.T) {
"VERTEXAI_PROJECT": "test-project",
"VERTEXAI_LOCATION": "us-central1",
})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
require.Equal(t, cfg.Providers.Len(), 1)
@@ -304,7 +304,7 @@ func TestConfig_configureProvidersVertexAIWithoutCredentials(t *testing.T) {
"GOOGLE_CLOUD_PROJECT": "test-project",
"GOOGLE_CLOUD_LOCATION": "us-central1",
})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
// Provider should not be configured without proper credentials
@@ -329,7 +329,7 @@ func TestConfig_configureProvidersVertexAIMissingProject(t *testing.T) {
"GOOGLE_GENAI_USE_VERTEXAI": "true",
"GOOGLE_CLOUD_LOCATION": "us-central1",
})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
// Provider should not be configured without project
@@ -353,7 +353,7 @@ func TestConfig_configureProvidersSetProviderID(t *testing.T) {
env := env.NewFromMap(map[string]string{
"OPENAI_API_KEY": "test-key",
})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
require.Equal(t, cfg.Providers.Len(), 1)
@@ -544,7 +544,7 @@ func TestConfig_configureProvidersWithDisabledProvider(t *testing.T) {
env := env.NewFromMap(map[string]string{
"OPENAI_API_KEY": "test-key",
})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
@@ -572,7 +572,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
require.NoError(t, err)
@@ -595,7 +595,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
require.NoError(t, err)
@@ -617,7 +617,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
require.NoError(t, err)
@@ -642,7 +642,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
require.NoError(t, err)
@@ -667,7 +667,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
require.NoError(t, err)
@@ -695,7 +695,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
require.NoError(t, err)
@@ -725,7 +725,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
require.NoError(t, err)
@@ -760,7 +760,7 @@ func TestConfig_configureProvidersEnhancedCredentialValidation(t *testing.T) {
env := env.NewFromMap(map[string]string{
"GOOGLE_GENAI_USE_VERTEXAI": "false",
})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
@@ -791,7 +791,7 @@ func TestConfig_configureProvidersEnhancedCredentialValidation(t *testing.T) {
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
@@ -822,7 +822,7 @@ func TestConfig_configureProvidersEnhancedCredentialValidation(t *testing.T) {
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
@@ -855,7 +855,7 @@ func TestConfig_configureProvidersEnhancedCredentialValidation(t *testing.T) {
env := env.NewFromMap(map[string]string{
"OPENAI_API_KEY": "test-key",
})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
@@ -889,7 +889,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {
cfg := &Config{}
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
@@ -925,7 +925,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {
cfg := &Config{}
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
@@ -955,7 +955,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {
cfg := &Config{}
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
_, _, err = cfg.defaultModelSelection(knownProviders)
@@ -998,7 +998,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {
}
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
large, small, err := cfg.defaultModelSelection(knownProviders)
@@ -1042,7 +1042,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {
}
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
_, _, err = cfg.defaultModelSelection(knownProviders)
@@ -1084,7 +1084,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {
}
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
large, small, err := cfg.defaultModelSelection(knownProviders)
@@ -1129,7 +1129,7 @@ func TestConfig_configureProvidersDisableDefaultProviders(t *testing.T) {
env := env.NewFromMap(map[string]string{
"OPENAI_API_KEY": "test-key",
})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.ErrorContains(t, err, "no custom providers")
@@ -1172,7 +1172,7 @@ func TestConfig_configureProvidersDisableDefaultProviders(t *testing.T) {
"MY_API_KEY": "test-key",
"OPENAI_API_KEY": "test-key",
})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
@@ -1226,7 +1226,7 @@ func TestConfig_configureProvidersDisableDefaultProviders(t *testing.T) {
"OPENAI_API_KEY": "test-key",
"ANTHROPIC_API_KEY": "test-key",
})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
@@ -1254,7 +1254,7 @@ func TestConfig_configureProvidersDisableDefaultProviders(t *testing.T) {
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
require.ErrorContains(t, err, "no custom providers")
@@ -1278,7 +1278,7 @@ func TestConfig_configureProvidersDisableDefaultProviders(t *testing.T) {
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})
require.ErrorContains(t, err, "no custom providers")
@@ -1336,7 +1336,7 @@ func TestConfig_configureSelectedModels(t *testing.T) {
cfg.setDefaults(dir, "")
store := &ConfigStore{config: cfg, globalDataPath: globalPath}
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(store, env, resolver, knownProviders)
require.NoError(t, err)
@@ -1386,7 +1386,7 @@ func TestConfig_configureSelectedModels(t *testing.T) {
}
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
@@ -1448,7 +1448,7 @@ func TestConfig_configureSelectedModels(t *testing.T) {
}
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
@@ -1493,7 +1493,7 @@ func TestConfig_configureSelectedModels(t *testing.T) {
}
cfg.setDefaults("/tmp", "")
env := env.NewFromMap(map[string]string{})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
@@ -1532,7 +1532,7 @@ func TestConfig_configureProviders_HyperAPIKeyFromEnv(t *testing.T) {
env := env.NewFromMap(map[string]string{
"HYPER_API_KEY": "env-api-key",
})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
require.Equal(t, 1, cfg.Providers.Len())
@@ -1579,7 +1579,7 @@ func TestConfig_configureProviders_HyperAPIKeyFromConfigOverrides(t *testing.T)
env := env.NewFromMap(map[string]string{
"HYPER_API_KEY": "env-api-key",
})
- resolver := NewEnvironmentVariableResolver(env)
+ resolver := NewShellVariableResolver(env)
err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)
require.NoError(t, err)
require.Equal(t, 1, cfg.Providers.Len())
@@ -1590,18 +1590,13 @@ func TestConfig_configureProviders_HyperAPIKeyFromConfigOverrides(t *testing.T)
require.Equal(t, "env-api-key", pc.APIKey)
}
-// TestConfig_configureProviders_ProviderHeaderResolveFailure pins the
-// intentional divergence at load.go:225: a provider header whose
-// expansion fails must NOT fail the whole config load. It logs an
-// error, keeps the literal template in the resolved header map, and
-// moves on. The MCP contract (hard error on failed expansion) does not
-// apply here because many providers ship DefaultHeaders that reference
-// env vars which are legitimately unset on most hosts.
-//
-// If this test ever flips to requiring an error, read PLAN.md "Design
-// decisions" item 4 before changing the production code — the
-// divergence is deliberate.
-func TestConfig_configureProviders_ProviderHeaderResolveFailure(t *testing.T) {
+// TestConfig_configureProviders_ProviderHeaderResolveError pins
+// Phase 2 design decision #14: a failing $(cmd) in a provider header
+// must fail the provider load with a clear message that names the
+// offending header. The Phase 1 log-and-continue divergence at
+// load.go:225 is gone; provider headers now share the MCP error
+// contract.
+func TestConfig_configureProviders_ProviderHeaderResolveError(t *testing.T) {
knownProviders := []catwalk.Provider{
{
ID: "openai",
@@ -1615,13 +1610,9 @@ func TestConfig_configureProviders_ProviderHeaderResolveFailure(t *testing.T) {
Providers: csync.NewMapFrom(map[string]ProviderConfig{
"openai": {
ExtraHeaders: map[string]string{
- // Failing $(...) — inner command exits 1, no stdout.
+ // Failing $(...) — inner command exits 1. Must
+ // propagate as an error, not a silent truncation.
"X-Broken": "$(false)",
- // Unset var — nounset makes this an error too.
- "X-Missing": "$PROVIDER_HEADER_NEVER_SET",
- // Happy path: must still be resolved, proving the
- // failure in the other two did not abort the loop.
- "X-Static": "kept-literal",
},
},
}),
@@ -1635,15 +1626,238 @@ func TestConfig_configureProviders_ProviderHeaderResolveFailure(t *testing.T) {
resolver := NewShellVariableResolver(testEnv)
err := cfg.configureProviders(testStore(cfg), testEnv, resolver, knownProviders)
- require.NoError(t, err, "provider load must tolerate failing header expansion")
+ require.Error(t, err, "failing $(cmd) in a header must fail the provider load")
+ require.Contains(t, err.Error(), "X-Broken", "error must name the offending header")
+}
+
+// TestConfig_configureProviders_CatwalkDefaultWithUnsetVarLoads pins
+// Phase 2 design decisions #11 and #18 from the provider angle: a
+// Catwalk-style default header like
+// "OpenAI-Organization": "$OPENAI_ORG_ID" must load cleanly under
+// lenient nounset (unset → "" → header dropped), not fail the load
+// and not leave the literal template on the wire.
+func TestConfig_configureProviders_CatwalkDefaultWithUnsetVarLoads(t *testing.T) {
+ knownProviders := []catwalk.Provider{
+ {
+ ID: "openai",
+ APIKey: "$OPENAI_API_KEY",
+ APIEndpoint: "https://api.openai.com/v1",
+ Models: []catwalk.Model{{ID: "test-model"}},
+ DefaultHeaders: map[string]string{
+ "OpenAI-Organization": "$OPENAI_ORG_ID",
+ },
+ },
+ }
+
+ cfg := &Config{}
+ cfg.setDefaults("/tmp", "")
+
+ testEnv := env.NewFromMap(map[string]string{
+ "OPENAI_API_KEY": "test-key",
+ "PATH": os.Getenv("PATH"),
+ })
+ resolver := NewShellVariableResolver(testEnv)
+
+ err := cfg.configureProviders(testStore(cfg), testEnv, resolver, knownProviders)
+ require.NoError(t, err, "optional env-gated header must not fail the load")
pc, ok := cfg.Providers.Get("openai")
require.True(t, ok, "openai provider must still be configured")
+ _, present := pc.ExtraHeaders["OpenAI-Organization"]
+ require.False(t, present, "header whose value resolves to empty must be absent")
+}
+
+// TestConfig_configureProviders_LiteralEmptyHeaderDropped pins design
+// decision #18 for the literal case: a user-authored
+// "X-Custom": "" in extra_headers is absent from the resolved map.
+// Applies to both known- and custom-provider paths; this test
+// exercises the custom-provider loop.
+func TestConfig_configureProviders_LiteralEmptyHeaderDropped(t *testing.T) {
+ cfg := &Config{
+ Providers: csync.NewMapFrom(map[string]ProviderConfig{
+ "my-llm": {
+ APIKey: "test-key",
+ BaseURL: "https://my-llm.example.com/v1",
+ Type: catwalk.TypeOpenAI,
+ Models: []catwalk.Model{{ID: "m"}},
+ ExtraHeaders: map[string]string{
+ "X-Custom": "",
+ "X-Kept": "present",
+ },
+ },
+ }),
+ }
+ cfg.setDefaults("/tmp", "")
+
+ testEnv := env.NewFromMap(map[string]string{
+ "PATH": os.Getenv("PATH"),
+ })
+ resolver := NewShellVariableResolver(testEnv)
+
+ err := cfg.configureProviders(testStore(cfg), testEnv, resolver, []catwalk.Provider{})
+ require.NoError(t, err)
+
+ pc, ok := cfg.Providers.Get("my-llm")
+ require.True(t, ok)
+ _, present := pc.ExtraHeaders["X-Custom"]
+ require.False(t, present, "literal empty-string header must be dropped")
+ require.Equal(t, "present", pc.ExtraHeaders["X-Kept"])
+}
+
+// TestConfig_configureProviders_EchoEmptyHeaderDropped pins design
+// decision #18 for the non-failing empty case: $(echo) exits 0 with
+// empty output, resolves cleanly to "", and must be dropped the same
+// way an unset bare $VAR is. Exercises the known-provider loop.
+func TestConfig_configureProviders_EchoEmptyHeaderDropped(t *testing.T) {
+ knownProviders := []catwalk.Provider{
+ {
+ ID: "openai",
+ APIKey: "$OPENAI_API_KEY",
+ APIEndpoint: "https://api.openai.com/v1",
+ Models: []catwalk.Model{{ID: "test-model"}},
+ DefaultHeaders: map[string]string{
+ "X-Empty": "$(echo)",
+ "X-Kept": "present",
+ },
+ },
+ }
+
+ cfg := &Config{}
+ cfg.setDefaults("/tmp", "")
+
+ testEnv := env.NewFromMap(map[string]string{
+ "OPENAI_API_KEY": "test-key",
+ "PATH": os.Getenv("PATH"),
+ })
+ resolver := NewShellVariableResolver(testEnv)
+
+ err := cfg.configureProviders(testStore(cfg), testEnv, resolver, knownProviders)
+ require.NoError(t, err)
+
+ pc, ok := cfg.Providers.Get("openai")
+ require.True(t, ok)
+ _, present := pc.ExtraHeaders["X-Empty"]
+ require.False(t, present, "$(echo) → empty → header must be dropped")
+ require.Equal(t, "present", pc.ExtraHeaders["X-Kept"])
+}
+
+// TestConfig_configureProviders_UnsetAPIKeySkipsProvider pins Phase 2
+// Step 12 / design decision #15: under the lenient-nounset shell
+// resolver, $UNSET_API_KEY expands to ("", nil) rather than ("", err),
+// and the existing `v == "" || err != nil` skip path at load.go:331
+// still drops the provider. The slog.Warn line is emitted on the same
+// path but is not asserted here — internal/config/load_test.go's
+// TestMain replaces the default slog handler with an io.Discard
+// writer, so capturing that log line would require mid-test handler
+// swapping and a sync.Mutex dance that adds more flake surface than
+// signal. The observable outcome (provider absent from the map) is
+// what downstream code — model picker, agent wiring — actually reads,
+// so that's what we pin.
+func TestConfig_configureProviders_UnsetAPIKeySkipsProvider(t *testing.T) {
+ knownProviders := []catwalk.Provider{
+ {
+ ID: "openai",
+ APIKey: "$SOMETHING_UNSET",
+ APIEndpoint: "https://api.openai.com/v1",
+ Models: []catwalk.Model{{ID: "test-model"}},
+ },
+ }
+
+ // Existing user config for this known provider so the load.go:332
+ // `if configExists` branch fires and actually calls Providers.Del.
+ // Without it the provider was never in the map to begin with and
+ // the test would pass trivially.
+ cfg := &Config{
+ Providers: csync.NewMapFrom(map[string]ProviderConfig{
+ "openai": {BaseURL: "custom-url"},
+ }),
+ }
+ cfg.setDefaults("/tmp", "")
+
+ testEnv := env.NewFromMap(map[string]string{
+ "PATH": os.Getenv("PATH"),
+ })
+ resolver := NewShellVariableResolver(testEnv)
+
+ err := cfg.configureProviders(testStore(cfg), testEnv, resolver, knownProviders)
+ require.NoError(t, err, "skip path must not surface as a load error")
+
+ require.Equal(t, 0, cfg.Providers.Len(), "provider with unset API key must be skipped")
+ _, exists := cfg.Providers.Get("openai")
+ require.False(t, exists)
+}
+
+// TestConfig_configureProviders_FailingAPIKeyCmdSkipsProvider pins
+// that the two failure modes for APIKey — ("", nil) from an unset var
+// under lenient nounset and ("", err) from a failing $(cmd) — are
+// equivalent for the skip outcome at load.go:331. The `v == "" ||
+// err != nil` check fires on either branch; this test locks in that
+// equivalence so a future refactor that splits the check into two
+// paths doesn't accidentally start propagating $(false) as a load
+// error while keeping unset-var as a silent skip (or vice versa).
+func TestConfig_configureProviders_FailingAPIKeyCmdSkipsProvider(t *testing.T) {
+ knownProviders := []catwalk.Provider{
+ {
+ ID: "openai",
+ APIKey: "$(false)",
+ APIEndpoint: "https://api.openai.com/v1",
+ Models: []catwalk.Model{{ID: "test-model"}},
+ },
+ }
+
+ cfg := &Config{
+ Providers: csync.NewMapFrom(map[string]ProviderConfig{
+ "openai": {BaseURL: "custom-url"},
+ }),
+ }
+ cfg.setDefaults("/tmp", "")
+
+ testEnv := env.NewFromMap(map[string]string{
+ "PATH": os.Getenv("PATH"),
+ })
+ resolver := NewShellVariableResolver(testEnv)
+
+ err := cfg.configureProviders(testStore(cfg), testEnv, resolver, knownProviders)
+ require.NoError(t, err, "failing $(cmd) in API key must skip provider, not fail load")
+
+ require.Equal(t, 0, cfg.Providers.Len(), "provider with failing $(cmd) API key must be skipped")
+ _, exists := cfg.Providers.Get("openai")
+ require.False(t, exists)
+}
+
+// TestConfig_configureProviders_UnsetAzureEndpointSkipsProvider pins
+// the same contract on the Azure path at load.go:287 — APIEndpoint is
+// the field that gates Azure and goes through the same
+// `v == "" || err != nil` skip check. Covered here so both branches
+// of the shared skip pattern (APIKey default path and APIEndpoint
+// Azure path) are tested; a future refactor that unifies them can
+// rely on these two tests to catch drift.
+func TestConfig_configureProviders_UnsetAzureEndpointSkipsProvider(t *testing.T) {
+ knownProviders := []catwalk.Provider{
+ {
+ ID: catwalk.InferenceProviderAzure,
+ APIKey: "test-key",
+ APIEndpoint: "$UNSET_AZURE_ENDPOINT",
+ Models: []catwalk.Model{{ID: "test-model"}},
+ },
+ }
+
+ cfg := &Config{
+ Providers: csync.NewMapFrom(map[string]ProviderConfig{
+ "azure": {BaseURL: ""},
+ }),
+ }
+ cfg.setDefaults("/tmp", "")
+
+ testEnv := env.NewFromMap(map[string]string{
+ "PATH": os.Getenv("PATH"),
+ })
+ resolver := NewShellVariableResolver(testEnv)
+
+ err := cfg.configureProviders(testStore(cfg), testEnv, resolver, knownProviders)
+ require.NoError(t, err)
- // Literal template preserved for the two failing headers; the
- // happy-path header is resolved through the shell (pass-through
- // for a literal value).
- require.Equal(t, "$(false)", pc.ExtraHeaders["X-Broken"])
- require.Equal(t, "$PROVIDER_HEADER_NEVER_SET", pc.ExtraHeaders["X-Missing"])
- require.Equal(t, "kept-literal", pc.ExtraHeaders["X-Static"])
+ require.Equal(t, 0, cfg.Providers.Len(), "azure provider with unset endpoint must be skipped")
+ _, exists := cfg.Providers.Get("azure")
+ require.False(t, exists)
}
@@ -44,14 +44,31 @@ func TestMCPConfig_ResolvedURL(t *testing.T) {
require.Equal(t, "https://mcp.example.com/events", got)
})
- t.Run("unset var is an error wrapping the template", func(t *testing.T) {
+ t.Run("unset var expands to empty under lenient default", func(t *testing.T) {
t.Parallel()
+ // Phase 2 defaults to nounset-off: bare $VAR on an unset
+ // variable expands to "" rather than erroring. Here the
+ // host collapses to empty, so the caller sees a malformed
+ // URL rather than a resolver error; that's the expected
+ // trade-off for making $OPTIONAL-style patterns work, and
+ // required-credential callers should use ${VAR:?msg}.
m := MCPConfig{Type: MCPHttp, URL: "https://$MCP_MISSING_HOST/api"}
+ got, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil)))
+ require.NoError(t, err, "unset var must not error under lenient default")
+ require.Equal(t, "https:///api", got)
+ })
+
+ t.Run("colon-question on unset var errors regardless of toggle", func(t *testing.T) {
+ t.Parallel()
+ // ${VAR:?msg} is the opt-in strictness mechanism; it must
+ // hard-error even with NoUnset off so required credentials
+ // surface at load time instead of shipping empty-host URLs
+ // to the transport layer.
+ m := MCPConfig{Type: MCPHttp, URL: "https://${MCP_MISSING_HOST:?set MCP_MISSING_HOST}/api"}
_, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil)))
require.Error(t, err)
require.Contains(t, err.Error(), "url:")
- require.Contains(t, err.Error(), "$MCP_MISSING_HOST")
- require.Contains(t, err.Error(), "unbound")
+ require.Contains(t, err.Error(), "set MCP_MISSING_HOST")
})
t.Run("failing command substitution is an error", func(t *testing.T) {
@@ -10,8 +10,7 @@ import (
)
// resolveTimeout bounds how long a single ResolveValue call may spend
-// inside shell expansion (including any command substitution). Matches
-// the timeout used by the previous hand-rolled parser.
+// inside shell expansion (including any command substitution).
const resolveTimeout = 5 * time.Minute
type VariableResolver interface {
@@ -58,8 +57,11 @@ type shellVariableResolver struct {
// NewShellVariableResolver returns a VariableResolver that delegates to
// the embedded shell (the same interpreter used by the bash tool and
// hooks). Supported constructs match shell.ExpandValue: $VAR, ${VAR},
-// ${VAR:-default}, $(command), quoting, and escapes. Unset variables are
-// an error; use ${VAR:-} to opt in to an empty fallback.
+// ${VAR:-default}, $(command), quoting, and escapes. Unset variables
+// expand to the empty string by default, matching bash; use
+// ${VAR:?message} to require a value and fail loudly when it is missing.
+// The stricter "unset is always an error" mode is gated globally by
+// shell.NoUnset.
func NewShellVariableResolver(e env.Env, opts ...ShellResolverOption) VariableResolver {
r := &shellVariableResolver{
env: e,
@@ -77,9 +79,12 @@ func NewShellVariableResolver(e env.Env, opts ...ShellResolverOption) VariableRe
// - $VAR and ${VAR} for environment variables.
// - ${VAR:-default} / ${VAR:+alt} / ${VAR:?msg} for defaulting.
//
-// Unset variables are a hard error (nounset), mirroring the historical
-// behaviour of this resolver: silently expanding an unset variable to the
-// empty string is exactly how broken credentials reach MCP servers.
+// Unset variables expand to the empty string by default, matching bash.
+// Command-substitution failures are always a hard error. Required
+// credentials should use ${VAR:?message} so a missing variable fails
+// loudly at load time instead of quietly resolving to empty. Global
+// strict mode is available via shell.NoUnset for callers that want the
+// old nounset-on behaviour back.
func (r *shellVariableResolver) ResolveValue(value string) (string, error) {
// Preserve the historical backward-compat contract: a lone "$" is a
// malformed config value, not a legal literal. The underlying shell
@@ -167,27 +172,3 @@ func scrubErrorMessage(s string) string {
}
return string(out)
}
-
-type environmentVariableResolver struct {
- env env.Env
-}
-
-func NewEnvironmentVariableResolver(env env.Env) VariableResolver {
- return &environmentVariableResolver{
- env: env,
- }
-}
-
-// ResolveValue resolves environment variables from the provided env.Env.
-func (r *environmentVariableResolver) ResolveValue(value string) (string, error) {
- if len(value) == 0 || value[0] != '$' {
- return value, nil
- }
-
- varName := value[1:]
- resolvedValue := r.env.Get(varName)
- if resolvedValue == "" {
- return "", fmt.Errorf("environment variable %q not set", varName)
- }
- return resolvedValue, nil
-}
@@ -159,23 +159,50 @@ func TestResolvedEnv_RealShell_Deterministic(t *testing.T) {
require.True(t, slices.IsSorted(got), "env slice must be sorted; got %v", got)
}
-// TestResolvedEnv_RealShell_NounsetRegression is the single most
-// important assertion in this file: an unset variable is an error, not
-// an empty expansion. Swapping the hand-rolled parser for mvdan's
-// default-expansion behaviour without nounset would re-introduce
-// Defect 1 via a typo'd variable name.
-func TestResolvedEnv_RealShell_NounsetRegression(t *testing.T) {
+// TestResolvedEnv_RealShell_UnsetExpandsEmpty pins Phase 2's lenient
+// default: an unset bare $VAR expands to the empty string, matching
+// bash. The silent-empty-credential class of bug that motivated Phase
+// 1's nounset-on default is already prevented by the pure-function
+// error-returning contract of ResolvedEnv, so we no longer rely on
+// nounset to catch typo'd variable names. Users who want strict
+// behaviour for a required credential opt in per-reference with
+// ${VAR:?msg}; see TestResolvedEnv_RealShell_ColonQuestionIsStrict.
+func TestResolvedEnv_RealShell_UnsetExpandsEmpty(t *testing.T) {
t.Parallel()
m := MCPConfig{Env: map[string]string{
- // Intentional typo: user meant $FORGEJO_TOKEN.
+ // Intentional typo: user meant $FORGEJO_TOKEN. Under Phase 2
+ // defaults this expands to "" rather than erroring, matching
+ // bash's behaviour on bare $VAR.
"FORGEJO_ACCESS_TOKEN": "$FORGJO_TOKEN",
}}
got, err := m.ResolvedEnv(realShellResolver(nil))
- require.Error(t, err, "unset var must not silently expand to empty")
+ require.NoError(t, err, "unset var must expand to empty, not error")
+ require.Equal(t, []string{"FORGEJO_ACCESS_TOKEN="}, got)
+}
+
+// TestResolvedEnv_RealShell_ColonQuestionIsStrict pins the opt-in
+// strictness contract: ${VAR:?msg} must hard-error when VAR is unset,
+// regardless of the global NoUnset toggle. This is the recommended
+// mechanism for required credentials under the lenient default, so a
+// future refactor that accidentally swallows ${VAR:?...} errors would
+// silently ship empty tokens to MCP servers again.
+func TestResolvedEnv_RealShell_ColonQuestionIsStrict(t *testing.T) {
+ t.Parallel()
+
+ m := MCPConfig{Env: map[string]string{
+ "FORGEJO_ACCESS_TOKEN": "${FORGJO_TOKEN:?set FORGJO_TOKEN}",
+ }}
+ got, err := m.ResolvedEnv(realShellResolver(nil))
+ require.Error(t, err, "${VAR:?msg} must error when VAR is unset")
require.Nil(t, got)
- require.Contains(t, err.Error(), "FORGEJO_ACCESS_TOKEN")
- require.Contains(t, err.Error(), "$FORGJO_TOKEN")
+ // The resolver wraps with the env key and the user-written
+ // template; the inner shell error carries the :? message so
+ // users learn which credential is missing and why.
+ msg := err.Error()
+ require.Contains(t, msg, "FORGEJO_ACCESS_TOKEN")
+ require.Contains(t, msg, "${FORGJO_TOKEN:?set FORGJO_TOKEN}")
+ require.Contains(t, msg, "set FORGJO_TOKEN")
}
// TestResolvedEnv_RealShell_FailureDetail pins that a failing inner
@@ -232,6 +259,71 @@ func TestResolvedHeaders_RealShell_FailurePreservesOriginal(t *testing.T) {
require.Equal(t, orig, m.Headers, "receiver Headers must be preserved")
}
+// TestResolvedHeaders_RealShell_DropEmpty pins Phase 2 design
+// decision #18 on the MCP side: a header whose value resolves to the
+// empty string is omitted from the returned map. Covers the three
+// ways a value can legitimately land on empty — unset bare $VAR
+// under lenient nounset, a literal "", and a non-failing command
+// whose stdout is empty — and also pins that a failing $(cmd) still
+// errors rather than silently dropping.
+func TestResolvedHeaders_RealShell_DropEmpty(t *testing.T) {
+ t.Parallel()
+
+ t.Run("unset $VAR is absent", func(t *testing.T) {
+ t.Parallel()
+ m := MCPConfig{Headers: map[string]string{
+ "X-Missing": "$MCP_HEADER_NEVER_SET",
+ "X-Kept": "present",
+ }}
+ got, err := m.ResolvedHeaders(realShellResolver(nil))
+ require.NoError(t, err)
+ _, present := got["X-Missing"]
+ require.False(t, present, "unset bare $VAR → empty → header dropped")
+ require.Equal(t, "present", got["X-Kept"])
+ })
+
+ t.Run("literal empty string is absent", func(t *testing.T) {
+ t.Parallel()
+ m := MCPConfig{Headers: map[string]string{
+ "X-Custom": "",
+ "X-Kept": "present",
+ }}
+ got, err := m.ResolvedHeaders(realShellResolver(nil))
+ require.NoError(t, err)
+ _, present := got["X-Custom"]
+ require.False(t, present, "literal empty-string header must be dropped")
+ require.Equal(t, "present", got["X-Kept"])
+ })
+
+ t.Run("$(echo) is absent", func(t *testing.T) {
+ t.Parallel()
+ m := MCPConfig{Headers: map[string]string{
+ "X-Empty": "$(echo)",
+ "X-Kept": "present",
+ }}
+ got, err := m.ResolvedHeaders(realShellResolver(nil))
+ require.NoError(t, err)
+ _, present := got["X-Empty"]
+ require.False(t, present, "$(echo) → empty → header dropped")
+ require.Equal(t, "present", got["X-Kept"])
+ })
+
+ t.Run("$(false) errors and does not mutate", func(t *testing.T) {
+ t.Parallel()
+ m := MCPConfig{Headers: map[string]string{
+ "X-Broken": "$(false)",
+ "X-Kept": "present",
+ }}
+ orig := maps.Clone(m.Headers)
+
+ got, err := m.ResolvedHeaders(realShellResolver(nil))
+ require.Error(t, err)
+ require.Empty(t, got, "map must be nil/empty on failure, not a partial")
+ require.Contains(t, err.Error(), "header X-Broken")
+ require.Equal(t, orig, m.Headers, "receiver Headers must be preserved")
+ })
+}
+
// TestResolvedArgs_RealShell exercises both success and failure for
// m.Args symmetrically with Env. Args are ordered so error messages
// must identify a positional index, not a key.
@@ -267,6 +359,118 @@ func TestResolvedArgs_RealShell(t *testing.T) {
})
}
+// TestLSPConfig_ResolvedArgs_RealShell exercises both success and
+// failure for l.Args symmetrically with MCP args. Args are ordered so
+// error messages must identify a positional index, not a key.
+func TestLSPConfig_ResolvedArgs_RealShell(t *testing.T) {
+ t.Parallel()
+
+ t.Run("success expands $VAR in each element", func(t *testing.T) {
+ t.Parallel()
+ l := LSPConfig{Args: []string{"--root", "$HOME", "--flag", "literal"}}
+ r := realShellResolver(map[string]string{"HOME": "/home/tester"})
+ got, err := l.ResolvedArgs(r)
+ require.NoError(t, err)
+ require.Equal(t, []string{"--root", "/home/tester", "--flag", "literal"}, got)
+ })
+
+ t.Run("failure identifies offending index", func(t *testing.T) {
+ t.Parallel()
+ l := LSPConfig{Args: []string{"--root", "$(false)"}}
+ orig := slices.Clone(l.Args)
+
+ got, err := l.ResolvedArgs(realShellResolver(nil))
+ require.Error(t, err)
+ require.Nil(t, got)
+ require.Contains(t, err.Error(), "arg 1")
+ require.Equal(t, orig, l.Args, "receiver Args must be preserved")
+ })
+
+ t.Run("nil args returns nil, no error", func(t *testing.T) {
+ t.Parallel()
+ l := LSPConfig{}
+ got, err := l.ResolvedArgs(realShellResolver(nil))
+ require.NoError(t, err)
+ require.Nil(t, got)
+ })
+}
+
+// TestLSPConfig_ResolvedEnv_RealShell pins the LSP env contract:
+// success expands $VAR, failure wraps with the key name, and the
+// receiver map is never mutated. The shape is map[string]string
+// (not the MCP []string form) because powernap.ClientConfig.Environment
+// takes a map directly.
+func TestLSPConfig_ResolvedEnv_RealShell(t *testing.T) {
+ t.Parallel()
+
+ t.Run("success expands $VAR", func(t *testing.T) {
+ t.Parallel()
+ l := LSPConfig{Env: map[string]string{"GOPATH": "$HOME/go"}}
+ r := realShellResolver(map[string]string{"HOME": "/home/tester"})
+ got, err := l.ResolvedEnv(r)
+ require.NoError(t, err)
+ require.Equal(t, map[string]string{"GOPATH": "/home/tester/go"}, got)
+ })
+
+ t.Run("failure identifies offending key", func(t *testing.T) {
+ t.Parallel()
+ l := LSPConfig{Env: map[string]string{
+ "GOPATH": "$(false)",
+ "OTHER": "literal",
+ }}
+ orig := maps.Clone(l.Env)
+
+ got, err := l.ResolvedEnv(realShellResolver(nil))
+ require.Error(t, err)
+ require.Nil(t, got)
+ require.Contains(t, err.Error(), `env "GOPATH"`)
+ require.Equal(t, orig, l.Env, "receiver Env must be preserved")
+ })
+
+ t.Run("idempotent and non-mutating", func(t *testing.T) {
+ t.Parallel()
+ l := LSPConfig{Env: map[string]string{
+ "A": "$(echo one)",
+ "B": "literal",
+ }}
+ orig := maps.Clone(l.Env)
+ r := realShellResolver(nil)
+
+ first, err := l.ResolvedEnv(r)
+ require.NoError(t, err)
+ second, err := l.ResolvedEnv(r)
+ require.NoError(t, err)
+ require.Equal(t, first, second)
+ require.Equal(t, orig, l.Env, "receiver Env must be preserved")
+ })
+}
+
+// TestLSPConfig_IdentityResolver pins the client-mode contract: both
+// ResolvedArgs and ResolvedEnv round-trip the template verbatim under
+// IdentityResolver and never error on unset variables. Local
+// expansion would double-expand when the server does its own — this
+// has to stay a pure pass-through.
+func TestLSPConfig_IdentityResolver(t *testing.T) {
+ t.Parallel()
+
+ l := LSPConfig{
+ Args: []string{"--root", "$LSP_ROOT", "$(vault read -f lsp)"},
+ Env: map[string]string{
+ "GOPATH": "$HOME/go",
+ "TOKEN": "$(cat /run/secrets/x)",
+ },
+ }
+ r := IdentityResolver()
+
+ args, err := l.ResolvedArgs(r)
+ require.NoError(t, err)
+ require.Equal(t, l.Args, args)
+
+ envs, err := l.ResolvedEnv(r)
+ require.NoError(t, err)
+ require.Equal(t, l.Env, envs)
+}
+
// TestMCPConfig_IdentityResolver pins the client-mode contract: every
// Resolved* method round-trips the template verbatim and never errors
// on unset variables. Local expansion would double-expand when the
@@ -220,62 +220,6 @@ func TestScrubErrorMessage(t *testing.T) {
})
}
-func TestEnvironmentVariableResolver_ResolveValue(t *testing.T) {
- tests := []struct {
- name string
- value string
- envVars map[string]string
- expected string
- expectError bool
- }{
- {
- name: "non-variable string returns as-is",
- value: "plain-string",
- expected: "plain-string",
- },
- {
- name: "environment variable resolution",
- value: "$HOME",
- envVars: map[string]string{"HOME": "/home/user"},
- expected: "/home/user",
- },
- {
- name: "environment variable with complex value",
- value: "$PATH",
- envVars: map[string]string{"PATH": "/usr/bin:/bin:/usr/local/bin"},
- expected: "/usr/bin:/bin:/usr/local/bin",
- },
- {
- name: "missing environment variable returns error",
- value: "$MISSING_VAR",
- envVars: map[string]string{},
- expectError: true,
- },
- {
- name: "empty environment variable returns error",
- value: "$EMPTY_VAR",
- envVars: map[string]string{"EMPTY_VAR": ""},
- expectError: true,
- },
- }
-
- for _, tt := range tests {
- t.Run(tt.name, func(t *testing.T) {
- testEnv := env.NewFromMap(tt.envVars)
- resolver := NewEnvironmentVariableResolver(testEnv)
-
- result, err := resolver.ResolveValue(tt.value)
-
- if tt.expectError {
- require.Error(t, err)
- } else {
- require.NoError(t, err)
- require.Equal(t, tt.expected, result)
- }
- })
- }
-}
-
func TestNewShellVariableResolver(t *testing.T) {
testEnv := env.NewFromMap(map[string]string{"TEST": "value"})
resolver := NewShellVariableResolver(testEnv)
@@ -283,11 +227,3 @@ func TestNewShellVariableResolver(t *testing.T) {
require.NotNil(t, resolver)
require.Implements(t, (*VariableResolver)(nil), resolver)
}
-
-func TestNewEnvironmentVariableResolver(t *testing.T) {
- testEnv := env.NewFromMap(map[string]string{"TEST": "value"})
- resolver := NewEnvironmentVariableResolver(testEnv)
-
- require.NotNil(t, resolver)
- require.Implements(t, (*VariableResolver)(nil), resolver)
-}
@@ -5,7 +5,6 @@ import (
"encoding/json"
"fmt"
"log/slog"
- "maps"
"os"
"path/filepath"
"slices"
@@ -169,11 +168,21 @@ func (c *Client) createPowernapClient() error {
return fmt.Errorf("invalid lsp command: %w", err)
}
+ args, err := c.config.ResolvedArgs(c.resolver)
+ if err != nil {
+ return fmt.Errorf("invalid lsp args: %w", err)
+ }
+
+ envs, err := c.config.ResolvedEnv(c.resolver)
+ if err != nil {
+ return fmt.Errorf("invalid lsp env: %w", err)
+ }
+
clientConfig := powernap.ClientConfig{
Command: home.Long(command),
- Args: c.config.Args,
+ Args: args,
RootURI: rootURI,
- Environment: maps.Clone(c.config.Env),
+ Environment: envs,
Settings: c.config.Options,
InitOptions: c.config.InitOptions,
WorkspaceFolders: []protocol.WorkspaceFolder{
@@ -26,7 +26,7 @@ func TestClient(t *testing.T) {
// Test creating a powernap client - this will likely fail with echo
// but we can still test the basic structure
- client, err := New(ctx, "test", cfg, config.NewEnvironmentVariableResolver(env.NewFromMap(map[string]string{
+ client, err := New(ctx, "test", cfg, config.NewShellVariableResolver(env.NewFromMap(map[string]string{
"THE_CMD": "echo",
})), ".", false)
if err != nil {
@@ -61,6 +61,42 @@ func TestClient(t *testing.T) {
}
}
+// TestNew_ExpansionFailure_Args pins that a failing $(cmd) in LSP
+// args surfaces as a load error prefixed "invalid lsp args:" and that
+// no client is returned. Mirrors the MCP contract where expansion
+// failure hard-stops transport creation rather than silently running
+// with an empty or literal value.
+func TestNew_ExpansionFailure_Args(t *testing.T) {
+ t.Parallel()
+
+ cfg := config.LSPConfig{
+ Command: "echo",
+ Args: []string{"--root", "$(false)"},
+ }
+ resolver := config.NewShellVariableResolver(env.NewFromMap(map[string]string{}))
+
+ client, err := New(t.Context(), "test-args-fail", cfg, resolver, ".", false)
+ require.Error(t, err)
+ require.Nil(t, client, "client must not start when args expansion fails")
+ require.Contains(t, err.Error(), "invalid lsp args")
+}
+
+// TestNew_ExpansionFailure_Env pins the same contract for env values.
+func TestNew_ExpansionFailure_Env(t *testing.T) {
+ t.Parallel()
+
+ cfg := config.LSPConfig{
+ Command: "echo",
+ Env: map[string]string{"BAD": "$(false)"},
+ }
+ resolver := config.NewShellVariableResolver(env.NewFromMap(map[string]string{}))
+
+ client, err := New(t.Context(), "test-env-fail", cfg, resolver, ".", false)
+ require.Error(t, err)
+ require.Nil(t, client, "client must not start when env expansion fails")
+ require.Contains(t, err.Error(), "invalid lsp env")
+}
+
func TestNilClient(t *testing.T) {
t.Parallel()
@@ -7,6 +7,7 @@ import (
"io"
"os"
"strings"
+ "sync/atomic"
"mvdan.cc/sh/v3/expand"
"mvdan.cc/sh/v3/interp"
@@ -18,11 +19,28 @@ import (
// to be embedded in a failing inner command.
const maxInnerStderrBytes = 512
+// NoUnset controls whether ExpandValue treats unset variables as an
+// error. Default false matches bash: $UNSET expands to "". Store true
+// to re-enable strict mode globally. Not exposed in crush.json; this is
+// an internal escape hatch in case the lenient default turns out to be
+// the wrong call.
+//
+// Declared atomic because ExpandValue is invoked concurrently (multiple
+// MCP / LSP / provider loads in flight at startup, hook execution, etc.)
+// and an unsynchronised read/write pair is a data race under the Go
+// memory model regardless of test-level happens-before reasoning. The
+// atomic load on the hot path is negligible against the cost of parsing
+// and running through mvdan.
+//
+// See PLAN.md Phase 2 design decisions #11 and #12 for the full
+// rationale.
+var NoUnset atomic.Bool
+
// ExpandValue expands shell-style substitutions in a single config value.
//
// Supported constructs match the bash tool:
//
-// - $VAR and ${VAR} (unset is an error; see nounset below).
+// - $VAR and ${VAR}.
// - ${VAR:-default} / ${VAR:+alt} / ${VAR:?msg}.
// - $(command) with full quoting and nesting.
// - escaped and quoted strings ("...", '...').
@@ -32,9 +50,11 @@ const maxInnerStderrBytes = 512
// - Returns exactly one string. No field splitting, no globbing, no
// pathname generation. Multi-word command output is preserved
// verbatim; it is never split into multiple values.
-// - Nounset is on: unset variables produce an error instead of
-// expanding to the empty string. Use ${VAR:-default} to opt in to
-// an empty fallback.
+// - Nounset is off by default, matching bash: unset variables expand
+// to "". Opt in to strict behaviour per-reference with
+// ${VAR:?msg}, which errors loudly when VAR is unset regardless of
+// the global toggle. Flip the global default via
+// shell.NoUnset.Store(true) as an internal escape hatch.
// - Embedded whitespace and newlines in the input are preserved
// verbatim. Command substitution strips trailing newlines only
// (POSIX), never leading or internal whitespace.
@@ -63,22 +83,27 @@ func ExpandValue(ctx context.Context, value string, env []string) (string, error
logger: noopLogger{},
}
+ strict := NoUnset.Load()
+
var stderrBuf bytes.Buffer
cfg := &expand.Config{
Env: expand.ListEnviron(env...),
- NoUnset: true,
+ NoUnset: strict,
CmdSubst: func(w io.Writer, cs *syntax.CmdSubst) error {
stderrBuf.Reset()
- runner, rerr := interp.New(
+ runnerOpts := []interp.RunnerOption{
interp.StdIO(nil, w, &stderrBuf),
interp.Interactive(false),
interp.Env(expand.ListEnviron(env...)),
interp.Dir(s.cwd),
interp.ExecHandlers(s.execHandlers()...),
+ }
+ if strict {
// Match the outer NoUnset: an unset $VAR inside
// $(...) is also an error, not a silent empty.
- interp.Params("-u"),
- )
+ runnerOpts = append(runnerOpts, interp.Params("-u"))
+ }
+ runner, rerr := interp.New(runnerOpts...)
if rerr != nil {
return rerr
}
@@ -105,22 +105,25 @@ func TestExpandValue_Success(t *testing.T) {
func TestExpandValue_Errors(t *testing.T) {
t.Parallel()
- t.Run("unset var is an error, not empty", func(t *testing.T) {
+ t.Run("unset var expands to empty under lenient default", func(t *testing.T) {
t.Parallel()
- _, err := ExpandValue(t.Context(), "$MISSING", nil)
- require.Error(t, err)
+ got, err := ExpandValue(t.Context(), "$MISSING", nil)
+ require.NoError(t, err)
+ require.Equal(t, "", got)
})
- t.Run("unset var inside braces is an error", func(t *testing.T) {
+ t.Run("unset var inside braces expands to empty", func(t *testing.T) {
t.Parallel()
- _, err := ExpandValue(t.Context(), "${MISSING}", nil)
- require.Error(t, err)
+ got, err := ExpandValue(t.Context(), "${MISSING}", nil)
+ require.NoError(t, err)
+ require.Equal(t, "", got)
})
- t.Run("unset var inside cmdsubst is an error", func(t *testing.T) {
+ t.Run("unset var inside cmdsubst expands to empty", func(t *testing.T) {
t.Parallel()
- _, err := ExpandValue(t.Context(), `$(printf '%s' "$MISSING")`, nil)
- require.Error(t, err)
+ got, err := ExpandValue(t.Context(), `$(printf '%s' "$MISSING")`, nil)
+ require.NoError(t, err)
+ require.Equal(t, "", got)
})
t.Run("bad syntax returns error", func(t *testing.T) {
@@ -173,6 +176,45 @@ func TestExpandValue_Errors(t *testing.T) {
})
}
+// TestExpandValue_StrictToggle pins the NoUnset escape hatch: when a
+// caller flips strict mode on, bare $UNSET must error instead of
+// expanding to the empty string. Must not run in parallel: it mutates
+// the package-level NoUnset atomic, so a parallel peer observing the
+// flipped value would break the lenient default other tests assume.
+func TestExpandValue_StrictToggle(t *testing.T) {
+ NoUnset.Store(true)
+ t.Cleanup(func() { NoUnset.Store(false) })
+
+ _, err := ExpandValue(t.Context(), "$UNSET", nil)
+ require.Error(t, err)
+
+ _, err = ExpandValue(t.Context(), "${UNSET}", nil)
+ require.Error(t, err)
+
+ _, err = ExpandValue(t.Context(), `$(printf '%s' "$UNSET")`, nil)
+ require.Error(t, err)
+}
+
+// TestExpandValue_RequiredOptIn pins the per-reference opt-in strict
+// idiom ${VAR:?msg}: it must error whether or not the global NoUnset
+// toggle is on, so config authors can mark individual credentials as
+// required without flipping the global default.
+func TestExpandValue_RequiredOptIn(t *testing.T) {
+ t.Parallel()
+
+ _, err := ExpandValue(t.Context(), "${REQUIRED:?must be set}", nil)
+ require.Error(t, err)
+ require.Contains(t, err.Error(), "must be set")
+
+ got, err := ExpandValue(
+ t.Context(),
+ "${REQUIRED:?must be set}",
+ []string{"REQUIRED=ok"},
+ )
+ require.NoError(t, err)
+ require.Equal(t, "ok", got)
+}
+
func TestSanitizeStderr(t *testing.T) {
t.Parallel()
@@ -29,6 +29,58 @@ Crush uses JSON configuration files with the following priority (highest to lowe
The `$schema` property enables IDE autocomplete but is optional.
+## Shell Expansion
+
+Crush runs selected string fields through an embedded bash-compatible
+shell at load time, so values can pull from env vars, files, or helper
+commands.
+
+Supported constructs (match the `bash` tool):
+
+- `$VAR` and `${VAR}`
+- `${VAR:-default}`, `${VAR:+alt}`, `${VAR:?message}`
+- `$(command)` with full quoting and nesting
+- Single- and double-quoted strings, escapes
+
+Default semantics match bash: an unset variable expands to an empty
+string, no error. A failing `$(command)` is always a hard error. For
+required credentials, use `${VAR:?message}` so a missing variable
+fails loudly at load time with your message.
+
+```json
+{ "api_key": "${CODEBERG_TOKEN:?set CODEBERG_TOKEN}" }
+```
+
+### Which fields expand
+
+| Surface | Expansion |
+| --------------------------------------------------- | --------- |
+| Provider `api_key`, `base_url`, `api_endpoint` | yes |
+| Provider `extra_headers` | yes |
+| Provider `extra_body` | **no** |
+| MCP `command`, `args`, `env`, `headers`, `url` | yes |
+| LSP `command`, `args`, `env` | yes |
+| Hook `command` | runs via `sh -c`, not the resolver |
+
+`extra_body` is a JSON passthrough. If you need env-driven values in
+a request body, put them in `extra_headers`, `api_key`, or
+`base_url` instead.
+
+### Empty-resolved headers are dropped
+
+When a header value resolves to the empty string (unset variable,
+`$(echo)`, or literal `""`), the header is omitted from the
+outgoing request. This keeps optional env-gated headers like
+`"OpenAI-Organization": "$OPENAI_ORG_ID"` working cleanly when the
+var isn't set. Applies to MCP `headers` and provider `extra_headers`.
+
+### Security note
+
+`crush.json` is trusted code. Any `$(...)` in it runs at load time
+with the invoking user's shell privileges, before the UI appears.
+Don't launch Crush in a directory whose `crush.json` you haven't
+reviewed.
+
## Common Tasks
- Add a custom provider: add an entry under `providers` with `type`, `base_url`, `api_key`, and `models`.
@@ -79,7 +131,8 @@ The `$schema` property enables IDE autocomplete but is optional.
```
- `type` (required): `openai`, `openai-compat`, or `anthropic`
-- `api_key` supports `$ENV_VAR` syntax.
+- `api_key`, `base_url`, `api_endpoint`, and `extra_headers` are shell-expanded (see [Shell Expansion](#shell-expansion)).
+- `extra_body` is a JSON passthrough and is **not** expanded.
- Additional fields: `disable`, `system_prompt_prefix`, `extra_headers`, `extra_body`, `provider_options`.
## LSP Configuration
@@ -89,7 +142,7 @@ The `$schema` property enables IDE autocomplete but is optional.
"lsp": {
"go": {
"command": "gopls",
- "env": { "GOTOOLCHAIN": "go1.24.5" }
+ "env": { "GOPATH": "$HOME/go" }
},
"typescript": {
"command": "typescript-language-server",
@@ -100,6 +153,7 @@ The `$schema` property enables IDE autocomplete but is optional.
```
- `command` (required), `args`, `env` cover most setups.
+- `command`, `args`, and `env` values are shell-expanded (see [Shell Expansion](#shell-expansion)).
- Additional fields: `disabled`, `filetypes`, `root_markers`, `init_options`, `options`, `timeout`.
## MCP Servers
@@ -124,6 +178,7 @@ The `$schema` property enables IDE autocomplete but is optional.
```
- `type` (required): `stdio`, `sse`, or `http`
+- `command`, `args`, `env`, `headers`, and `url` are shell-expanded (see [Shell Expansion](#shell-expansion)).
- Additional fields: `env`, `disabled`, `disabled_tools`, `timeout`.
## Options