From abacef933667599de956fc9493166bf9f92c56f6 Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Sun, 3 May 2026 17:02:26 -0400 Subject: [PATCH 01/10] shell: switch config value expansion to lenient by default Missing env vars in crush.json used to fail loudly. They now quietly expand to an empty string, the same as bash. If you want the old strict behavior for a specific value, write it as ${VAR:?message} and Crush will still complain when the variable is not set. Co-Authored-By: Charm Crush --- internal/shell/expand.go | 41 +++++++++++++++++++----- internal/shell/expand_test.go | 60 +++++++++++++++++++++++++++++------ 2 files changed, 84 insertions(+), 17 deletions(-) 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() From c3cf9622b20df3b47b40ff52d7a23c7d56910616 Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Sun, 3 May 2026 17:02:26 -0400 Subject: [PATCH 02/10] test: update config tests to match lenient expansion Updates tests that assumed missing variables in config values were an error. They now expect an empty string, matching the new default. New tests cover ${VAR:?message}, which is the way to still fail loudly when a variable is required. Co-Authored-By: Charm Crush --- internal/config/load_test.go | 21 ++++++++--- internal/config/mcp_resolved_url_test.go | 23 ++++++++++-- internal/config/resolve_real_test.go | 47 +++++++++++++++++++----- 3 files changed, 72 insertions(+), 19 deletions(-) diff --git a/internal/config/load_test.go b/internal/config/load_test.go index 80551a7749014b6f3262d4b0f2b70c26aabde34d..98c00bc553c6468e1663d43fbbc634c82931bf7e 100644 --- a/internal/config/load_test.go +++ b/internal/config/load_test.go @@ -1617,10 +1617,14 @@ func TestConfig_configureProviders_ProviderHeaderResolveFailure(t *testing.T) { ExtraHeaders: map[string]string{ // Failing $(...) — inner command exits 1, no stdout. "X-Broken": "$(false)", - // Unset var — nounset makes this an error too. + // Unset var — under Phase 2 lenient nounset this + // resolves cleanly to "" rather than erroring; + // Step 10 will rewrite this whole test once the + // empty-drop rule replaces the log-and-continue + // loop at load.go:225. "X-Missing": "$PROVIDER_HEADER_NEVER_SET", // Happy path: must still be resolved, proving the - // failure in the other two did not abort the loop. + // failure in X-Broken did not abort the loop. "X-Static": "kept-literal", }, }, @@ -1640,10 +1644,15 @@ func TestConfig_configureProviders_ProviderHeaderResolveFailure(t *testing.T) { pc, ok := cfg.Providers.Get("openai") require.True(t, ok, "openai provider must still be configured") - // Literal template preserved for the two failing headers; the - // happy-path header is resolved through the shell (pass-through - // for a literal value). + // X-Broken still errors on $(false) so its literal template is + // preserved by the log-and-continue loop. X-Missing resolves + // cleanly to "" under lenient nounset: the current divergence + // only kicks in on resolver errors, so a non-erroring empty + // expansion writes through. The happy-path header is resolved + // through the shell (pass-through for a literal value). Step 10 + // will flip this whole surface to the MCP contract plus the + // empty-drop rule from design decision #18. require.Equal(t, "$(false)", pc.ExtraHeaders["X-Broken"]) - require.Equal(t, "$PROVIDER_HEADER_NEVER_SET", pc.ExtraHeaders["X-Missing"]) + require.Equal(t, "", pc.ExtraHeaders["X-Missing"]) require.Equal(t, "kept-literal", pc.ExtraHeaders["X-Static"]) } 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_real_test.go b/internal/config/resolve_real_test.go index f33435e9615f44a09c9e2ce1692c3b3aa3ebf276..9c9ddd951d3b6ea76be0b92f099e5fcbaa17bd84 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 From f23e9982cc216751e509e15b9a147477070478bb Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Sun, 3 May 2026 17:02:26 -0400 Subject: [PATCH 03/10] config: fail provider header expansion loudly and drop empty values Provider extra_headers used to log and keep the raw template when a command inside the header failed. A failing command now stops the provider from loading, with a clear error that names the provider and the header. Headers whose value ends up as an empty string are dropped from the outgoing request, so "OpenAI-Organization": "$OPENAI_ORG_ID" keeps working when the variable is not set. MCP headers follow the same rule. Co-Authored-By: Charm Crush --- internal/config/config.go | 10 ++ internal/config/load.go | 25 +++-- internal/config/load_test.go | 152 +++++++++++++++++++++------ internal/config/resolve_real_test.go | 65 ++++++++++++ 4 files changed, 210 insertions(+), 42 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 78aafbfdbf48e39b791824fd56bfb32c2b8c6439..ce4dfa0d32969f6765f785919c1130096f614209 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -368,6 +368,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 { @@ -386,6 +393,9 @@ 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 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 98c00bc553c6468e1663d43fbbc634c82931bf7e..653291539709353480a81011b3012c3f7d36c36a 100644 --- a/internal/config/load_test.go +++ b/internal/config/load_test.go @@ -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,17 +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 — under Phase 2 lenient nounset this - // resolves cleanly to "" rather than erroring; - // Step 10 will rewrite this whole test once the - // empty-drop rule replaces the log-and-continue - // loop at load.go:225. - "X-Missing": "$PROVIDER_HEADER_NEVER_SET", - // Happy path: must still be resolved, proving the - // failure in X-Broken did not abort the loop. - "X-Static": "kept-literal", }, }, }), @@ -1639,20 +1626,117 @@ 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", "") - // X-Broken still errors on $(false) so its literal template is - // preserved by the log-and-continue loop. X-Missing resolves - // cleanly to "" under lenient nounset: the current divergence - // only kicks in on resolver errors, so a non-erroring empty - // expansion writes through. The happy-path header is resolved - // through the shell (pass-through for a literal value). Step 10 - // will flip this whole surface to the MCP contract plus the - // empty-drop rule from design decision #18. - require.Equal(t, "$(false)", pc.ExtraHeaders["X-Broken"]) - require.Equal(t, "", pc.ExtraHeaders["X-Missing"]) - require.Equal(t, "kept-literal", pc.ExtraHeaders["X-Static"]) + 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"]) } diff --git a/internal/config/resolve_real_test.go b/internal/config/resolve_real_test.go index 9c9ddd951d3b6ea76be0b92f099e5fcbaa17bd84..bb7083346b8cffbe44e371e481d73be34b84c5b2 100644 --- a/internal/config/resolve_real_test.go +++ b/internal/config/resolve_real_test.go @@ -259,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. From 1f83559440dda9ef8654c1fe3cc8e48f98c143dd Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Sun, 3 May 2026 17:02:26 -0400 Subject: [PATCH 04/10] lsp: expand shell variables in args and env LSP args and env values used to be passed to the language server as raw strings, so "$HOME/go" was sent literally. They now go through the same shell expansion already used elsewhere in the config. If expansion fails, that LSP fails to load with a message that points at the specific arg or env key. Co-Authored-By: Charm Crush --- internal/config/config.go | 71 +++++++++++++++++ internal/config/resolve_real_test.go | 112 +++++++++++++++++++++++++++ internal/lsp/client.go | 15 +++- internal/lsp/client_test.go | 36 +++++++++ 4 files changed, 231 insertions(+), 3 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index ce4dfa0d32969f6765f785919c1130096f614209..17fceeb660a090601c3f67136c2c5d4ed50cdf46 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -401,6 +401,77 @@ func (m MCPConfig) ResolvedHeaders(r VariableResolver) (map[string]string, error 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 +} + type Agent struct { ID string `json:"id,omitempty"` Name string `json:"name,omitempty"` diff --git a/internal/config/resolve_real_test.go b/internal/config/resolve_real_test.go index bb7083346b8cffbe44e371e481d73be34b84c5b2..64cecc5769c6574975ae38ecdafd982eadbbe252 100644 --- a/internal/config/resolve_real_test.go +++ b/internal/config/resolve_real_test.go @@ -359,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/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..0337dc8b9ba506264e519e1c53a725ab9fec35b1 100644 --- a/internal/lsp/client_test.go +++ b/internal/lsp/client_test.go @@ -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() From 71aa8d1382ecc05c9e7bb4a915e0c10917047f92 Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Sun, 3 May 2026 17:02:26 -0400 Subject: [PATCH 05/10] test: pin provider skip behavior for api_key and endpoint expansion Pins down that a provider with a missing or broken api_key is still skipped with a warning, now that unset variables quietly expand to empty instead of erroring. Azure-style api_endpoint is covered the same way. Tests only, no behavior change. Co-Authored-By: Charm Crush --- internal/config/load_test.go | 121 +++++++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+) diff --git a/internal/config/load_test.go b/internal/config/load_test.go index 653291539709353480a81011b3012c3f7d36c36a..b6038e0d0e3fb76e4567b4c5c739eb8da952311e 100644 --- a/internal/config/load_test.go +++ b/internal/config/load_test.go @@ -1740,3 +1740,124 @@ func TestConfig_configureProviders_EchoEmptyHeaderDropped(t *testing.T) { 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) + + require.Equal(t, 0, cfg.Providers.Len(), "azure provider with unset endpoint must be skipped") + _, exists := cfg.Providers.Get("azure") + require.False(t, exists) +} From bcce66238bfe697f6ff8aef71197d8e4b19f426e Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Sun, 3 May 2026 17:02:26 -0400 Subject: [PATCH 06/10] docs: document lenient shell expansion and security model Updates the README and a handful of struct docs to describe the new behavior: missing variables expand to empty, ${VAR:?message} is the way to require a value, empty header values are dropped, extra_body is a plain JSON passthrough, and crush.json should be treated as trusted code because any $(...) in it runs at load time with the user's shell privileges. Co-Authored-By: Charm Crush --- README.md | 31 +++++++++++++++++++++++++------ internal/config/config.go | 24 +++++++++++++++++++++--- 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 73997cd325f298d3fb8bce817baeee6fa4669fa6..29201395790cd35b4f2c925075978ff8226c01d5 100644 --- a/README.md +++ b/README.md @@ -297,12 +297,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/config/config.go b/internal/config/config.go index 17fceeb660a090601c3f67136c2c5d4ed50cdf46..8514ea7c03ed6e75dddc76250b1c831362bfd80c 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"` @@ -174,7 +187,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"` } From a28aded746952db21274b1c5222438959fa0f31c Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Sun, 3 May 2026 17:02:26 -0400 Subject: [PATCH 07/10] test: realign MCP init tests with lenient shell expansion Several MCP startup tests were written against the old "unset variable is an error" behavior and no longer tripped now that missing variables quietly expand to empty. The fixtures switch to $(false) or an empty URL so the failing and empty paths are still covered, and a couple of new subtests pin the new quiet behavior so it cannot silently regress. Co-Authored-By: Charm Crush --- internal/agent/tools/mcp/init_test.go | 126 +++++++++++++++++++++++--- 1 file changed, 112 insertions(+), 14 deletions(-) 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", }, From 45f7484b495a6ec4e155d38a82a9935bdb11d8a5 Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Sun, 3 May 2026 17:02:26 -0400 Subject: [PATCH 08/10] docs: update resolver godoc to match lenient default The godoc on the shell variable resolver still said unset variables were a hard error. Updated to match the new lenient default and to point at ${VAR:?message} as the way to require a value. Co-Authored-By: Charm Crush --- internal/config/resolve.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/internal/config/resolve.go b/internal/config/resolve.go index 8c22a8abc7a516cd19be6fc32eaa101d60e416d4..8e13e16d440a31b73672ab55a467cc7ff866a1b8 100644 --- a/internal/config/resolve.go +++ b/internal/config/resolve.go @@ -58,8 +58,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 +80,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 From 06f43507a3254ef30f09674003000f42e6c22b0b Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Sun, 3 May 2026 17:02:26 -0400 Subject: [PATCH 09/10] docs(skill): document shell expansion in crush-config skill Adds a Shell Expansion section to the skill that covers the supported syntax, the quiet-unset default, which fields expand (LSP args and env and MCP url included), that extra_body does not expand, the empty-header drop rule, and a short security note. The provider, LSP, and MCP subsections now link to that section instead of suggesting $ENV_VAR is the only option. Co-Authored-By: Charm Crush --- internal/skills/builtin/crush-config/SKILL.md | 59 ++++++++++++++++++- 1 file changed, 57 insertions(+), 2 deletions(-) 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 From f9134777c941001444eb57ecc81bfcb49c6366f5 Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Sun, 3 May 2026 17:02:26 -0400 Subject: [PATCH 10/10] config: remove unused environment variable resolver The hand-rolled $VARNAME-only resolver (NewEnvironmentVariableResolver) had no production callers. Tests have been switched to the shell resolver, which is what production already uses, and the dead type plus its tests are gone. Co-Authored-By: Charm Crush --- internal/config/load_test.go | 78 ++++++++++++++++----------------- internal/config/resolve.go | 27 +----------- internal/config/resolve_test.go | 64 --------------------------- internal/lsp/client_test.go | 2 +- 4 files changed, 41 insertions(+), 130 deletions(-) diff --git a/internal/config/load_test.go b/internal/config/load_test.go index b6038e0d0e3fb76e4567b4c5c739eb8da952311e..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()) diff --git a/internal/config/resolve.go b/internal/config/resolve.go index 8e13e16d440a31b73672ab55a467cc7ff866a1b8..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 { @@ -173,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_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_test.go b/internal/lsp/client_test.go index 0337dc8b9ba506264e519e1c53a725ab9fec35b1..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 {