diff --git a/README.md b/README.md index 7aed61d09a6e07cc2a9402cf7efd845a88919f6a..2a0c2ce75e58f307b52f073e263089b024724c3a 100644 --- a/README.md +++ b/README.md @@ -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 { diff --git a/internal/agent/tools/mcp/init_test.go b/internal/agent/tools/mcp/init_test.go index 49ceb0410931fb6d20531f440960fb226ab2d768..5e906054f649aa90083fcbb120cff556d51dd06c 100644 --- a/internal/agent/tools/mcp/init_test.go +++ b/internal/agent/tools/mcp/init_test.go @@ -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", }, diff --git a/internal/config/config.go b/internal/config/config.go index eeb6921c59af3ca960d1360ddff39b2b2babe7a3..b7f612a9f28e88e8ff9c6e430c3f471b401ebe57 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -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 diff --git a/internal/config/load.go b/internal/config/load.go index b10d6fa643a54ed0ad64174cea92bebd6284a7d8..367decab920d584de1fadfd377a78e43dd67dfe6 100644 --- a/internal/config/load.go +++ b/internal/config/load.go @@ -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 diff --git a/internal/config/load_test.go b/internal/config/load_test.go index 80551a7749014b6f3262d4b0f2b70c26aabde34d..61054e5ccbc9031702f3d1d778634bf53b625bcb 100644 --- a/internal/config/load_test.go +++ b/internal/config/load_test.go @@ -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) } diff --git a/internal/config/mcp_resolved_url_test.go b/internal/config/mcp_resolved_url_test.go index 97c0d9df4725164a0ca05933da5df35239fdb691..a1f262f2224b53ed8e7dd1f1044152b853ba90db 100644 --- a/internal/config/mcp_resolved_url_test.go +++ b/internal/config/mcp_resolved_url_test.go @@ -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) { diff --git a/internal/config/resolve.go b/internal/config/resolve.go index 8c22a8abc7a516cd19be6fc32eaa101d60e416d4..a4a359be2e4ce1f6be32eba99b1b68eb3f61fe25 100644 --- a/internal/config/resolve.go +++ b/internal/config/resolve.go @@ -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 -} diff --git a/internal/config/resolve_real_test.go b/internal/config/resolve_real_test.go index f33435e9615f44a09c9e2ce1692c3b3aa3ebf276..64cecc5769c6574975ae38ecdafd982eadbbe252 100644 --- a/internal/config/resolve_real_test.go +++ b/internal/config/resolve_real_test.go @@ -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 diff --git a/internal/config/resolve_test.go b/internal/config/resolve_test.go index 18b461b8a6bc88f73d6db935eebd302cc3514781..3d364e523ae4738d23888941bc429d2c00241fa2 100644 --- a/internal/config/resolve_test.go +++ b/internal/config/resolve_test.go @@ -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) -} diff --git a/internal/lsp/client.go b/internal/lsp/client.go index 57bb7ab968bf4e0bddc8a2bcc74ac4515e84f5c6..9f6f9885e4bcc6ff0ce303ed98e6c73d3502309f 100644 --- a/internal/lsp/client.go +++ b/internal/lsp/client.go @@ -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{ diff --git a/internal/lsp/client_test.go b/internal/lsp/client_test.go index 904a7640bc263acb3e41714de13fda637d061892..583f7a8962600d711ab5087a7edf99d22eedba6b 100644 --- a/internal/lsp/client_test.go +++ b/internal/lsp/client_test.go @@ -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() diff --git a/internal/shell/expand.go b/internal/shell/expand.go index ee110a1df7b6b60b9ac2d51b7ae5f6abcbf3ac79..cb35ed2aade3006da80b1eb96e7496df0d37b343 100644 --- a/internal/shell/expand.go +++ b/internal/shell/expand.go @@ -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 } diff --git a/internal/shell/expand_test.go b/internal/shell/expand_test.go index 8661d6682370d31666185576db7edddb17a65b97..77d04454abf4447405e5b319b785dcff6e9fc760 100644 --- a/internal/shell/expand_test.go +++ b/internal/shell/expand_test.go @@ -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() diff --git a/internal/skills/builtin/crush-config/SKILL.md b/internal/skills/builtin/crush-config/SKILL.md index 7e5a354ad43266713f79e05264ecabecae6ef1af..df4f6444b4145bcbd1d1de9a2c078aedb5b105e9 100644 --- a/internal/skills/builtin/crush-config/SKILL.md +++ b/internal/skills/builtin/crush-config/SKILL.md @@ -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