8f2fd3d · crush · git.iko.soy

git.iko.soy/crush

Merge pull request #2788 from charmbracelet/revolve-env-vars-round-two

config: lenient shell expansion default, uniform coverage across MCP, LSP, and providers

commit8f2fd3d5addea4449bf4a04c291c12bad5faab52 authorChristian Rocha <christian@rocha.is> dateSat 9 May 2026, 20:23 (5 months ago) parentb7607a0 f913477

14 files changed+970−226

compare with main

unifiedsplit

README.md+25−6 internal/agent/tools/mcp/init_test.go+112−14 internal/config/config.go+102−3 internal/config/load.go+17−8 internal/config/load_test.go+278−64 internal/config/mcp_resolved_url_test.go+20−3 internal/config/resolve.go+12−31 internal/config/resolve_real_test.go+214−10 internal/config/resolve_test.go−64 internal/lsp/client.go+12−3 internal/lsp/client_test.go+37−1 internal/shell/expand.go+33−8 internal/shell/expand_test.go+51−9 internal/skills/builtin/crush-config/SKILL.md+57−2

README.md+25−6

@@ -298,12 +298,31 @@ types: `stdio` for command-line servers, `http` for HTTP endpoints, and `sse`

298298for Server-Sent Events.

299299

300300Shell-style value expansion (`$VAR`, `${VAR:-default}`, `$(command)`, quoting,

301−and nesting (works in `command`, `args`, `env`, `headers`, and `url`, so

302−file-based secrets like work out of the box, so you can use values like

303−"$TOKEN"` and `"$(cat /path/to/secret/token)"``. Expansion runs through Crush's

304−embedded shell, so the same syntax works on all supported systems, including

305−Windows. Unset variables are an error; use `${VAR:-fallback}` to opt in to

306−a default.

301+nesting) works in `command`, `args`, `env`, `headers`, and `url`, so

302+file-based secrets work out of the box. You can use values like `"$TOKEN"`

303+or `"$(cat /path/to/secret/token)"`. Expansion runs through Crush's embedded

304+shell, so the same syntax works on every supported system, Windows included.

305+

306+Unset variables expand to the empty string by default, matching bash. For

307+required credentials, use `${VAR:?message}` so an unset variable fails loudly

308+at load time with `message` instead of silently resolving to empty:

309+

310+```json

311+{ "api_key": "${CODEBERG_TOKEN:?set CODEBERG_TOKEN}" }

312+```

313+

314+Headers (both MCP `headers` and provider `extra_headers`) whose value

315+resolves to the empty string are dropped from the outgoing request rather

316+than sent as `Header:`. That keeps optional env-gated headers like

317+`"OpenAI-Organization": "$OPENAI_ORG_ID"` clean when the variable is unset.

318+

319+Provider `extra_body` is a non-expanding JSON passthrough; put env-driven

320+values in `extra_headers` or the provider's `api_key` / `base_url`, all of

321+which do expand.

322+

323+> **Security note:** `crush.json` is trusted code. Any `$(...)` in it runs at

324+> load time with your shell's privileges, before the UI appears. Don't launch

325+> Crush in a directory whose `crush.json` you haven't reviewed.

307326

308327```json

309328{

internal/agent/tools/mcp/init_test.go+112−14

@@ -91,17 +91,40 @@ func TestCreateTransport_URLResolution(t *testing.T) {

9191 require.Equal(t, "https://mcp.example.com/events", sse.Endpoint)

9292 })

9393

94− t.Run("http unset var surfaces error, no transport created", func(t *testing.T) {

94+ t.Run("http failing $(cmd) surfaces error, no transport created", func(t *testing.T) {

9595 t.Parallel()

96+ // Under lenient nounset, unset $VAR expands to "" silently,

97+ // so the only way a URL resolution *errors* is a failing

98+ // $(cmd). Mirror the SSE subtest so both transports share

99+ // coverage for the url-resolve-failure path.

96100 m := config.MCPConfig{

97101 Type: config.MCPHttp,

98− URL: "https://$MCP_MISSING_HOST/api",

102+ URL: "https://$(false)/api",

99103 }

100− tr, err := createTransport(t.Context(), m, shell)

104+ tr, err := createTransport(t.Context(), m, shellResolverWithPath(t, nil))

101105 require.Error(t, err)

102106 require.Nil(t, tr)

103107 require.Contains(t, err.Error(), "url:")

104− require.Contains(t, err.Error(), "$MCP_MISSING_HOST")

108+ require.Contains(t, err.Error(), "$(false)")

109+ })

110+

111+ t.Run("http unset var expands empty", func(t *testing.T) {

112+ t.Parallel()

113+ // Pinning test for the new lenient-nounset default: an

114+ // unset bare $VAR in the URL is *not* an error. It

115+ // expands to "" and, here, leaves a syntactically weird

116+ // but non-empty URL that the existing non-empty guard

117+ // still lets through. Guards against a future regression

118+ // that flips strict-by-default back on.

119+ m := config.MCPConfig{

120+ Type: config.MCPHttp,

121+ URL: "https://$MCP_MISSING_HOST/api",

122+ }

123+ tr, err := createTransport(t.Context(), m, shell)

124+ require.NoError(t, err)

125+ sct, ok := tr.(*mcp.StreamableClientTransport)

126+ require.True(t, ok)

127+ require.Equal(t, "https:///api", sct.Endpoint)

105128 })

106129

107130 t.Run("sse failing $(cmd) surfaces error, no transport created", func(t *testing.T) {

@@ -201,16 +224,18 @@ func TestCreateTransport_StdioResolution(t *testing.T) {

201224 require.Contains(t, err.Error(), "env TOKEN")

202225 })

203226

204− t.Run("unset env var is a hard error, not silent empty", func(t *testing.T) {

227+ t.Run("failing env command is a hard error", func(t *testing.T) {

205228 t.Parallel()

206− // The regression at the heart of PLAN.md: unset vars used to

207− // silently substitute "" and hand an empty credential to the

208− // child process. Now they must error out before exec.

229+ // Under lenient nounset a bare $UNSET expands to ""

230+ // silently — see the pinning subtest below. The remaining

231+ // failure mode for env resolution is a $(cmd) that exits

232+ // non-zero, which must still error out and prevent exec so

233+ // we never hand a broken credential to the child process.

209234 r := shellResolverWithPath(t, nil)

210235 m := config.MCPConfig{

211236 Type: config.MCPStdio,

212237 Command: "forgejo-mcp",

213− Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$FORGJO_TOKEN"},

238+ Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$(exit 5)"},

214239 }

215240 tr, err := createTransport(t.Context(), m, r)

216241 require.Error(t, err)

@@ -218,6 +243,28 @@ func TestCreateTransport_StdioResolution(t *testing.T) {

218243 require.Contains(t, err.Error(), "env FORGEJO_ACCESS_TOKEN")

219244 })

220245

246+ t.Run("unset env var expands empty", func(t *testing.T) {

247+ t.Parallel()

248+ // Pinning test for the lenient-nounset default: a bare

249+ // $UNSET in an env value expands to "" without error, and

250+ // the empty entry is kept on the resulting exec.Cmd (env

251+ // entries, unlike headers, are not dropped — see design

252+ // decision #18). Guards against a regression that flips

253+ // strict-by-default back on and silently breaks users

254+ // with configs like FORGEJO_ACCESS_TOKEN=$FORGEJO_TOKEN.

255+ r := shellResolverWithPath(t, nil)

256+ m := config.MCPConfig{

257+ Type: config.MCPStdio,

258+ Command: "forgejo-mcp",

259+ Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$FORGEJO_TOKEN_UNSET"},

260+ }

261+ tr, err := createTransport(t.Context(), m, r)

262+ require.NoError(t, err)

263+ ct, ok := tr.(*mcp.CommandTransport)

264+ require.True(t, ok)

265+ require.Contains(t, ct.Command.Env, "FORGEJO_ACCESS_TOKEN=")

266+ })

267+

221268 t.Run("args resolution failure surfaces error, no transport created", func(t *testing.T) {

222269 t.Parallel()

223270 r := shellResolverWithPath(t, nil)

@@ -311,17 +358,43 @@ func TestCreateTransport_HeadersResolution(t *testing.T) {

311358

312359 t.Run("sse failing header surfaces error, no transport", func(t *testing.T) {

313360 t.Parallel()

361+ // Under lenient nounset a bare $MISSING expands to "",

362+ // which ResolvedHeaders drops — no error. The failing

363+ // $(cmd) path is the remaining way this can fail loudly;

364+ // cover it on the SSE branch to mirror the HTTP subtest.

314365 r := shellResolverWithPath(t, nil)

315366 m := config.MCPConfig{

316367 Type: config.MCPSSE,

317368 URL: "https://mcp.example.com/events",

318− Headers: map[string]string{"Authorization": "Bearer $MISSING_TOKEN"},

369+ Headers: map[string]string{"Authorization": "$(false)"},

319370 }

320371 tr, err := createTransport(t.Context(), m, r)

321372 require.Error(t, err)

322373 require.Nil(t, tr)

323374 require.Contains(t, err.Error(), "header Authorization")

324− require.Contains(t, err.Error(), "$MISSING_TOKEN")

375+ })

376+

377+ t.Run("sse unset var header drops silently", func(t *testing.T) {

378+ t.Parallel()

379+ // Pinning test for design decision #18 + lenient nounset:

380+ // a header whose value resolves to "" (here because the

381+ // bare $VAR is unset) is omitted from the round tripper

382+ // rather than sent as "X-Header:". Guards against a

383+ // regression that either re-introduces strict-by-default

384+ // or stops dropping empty headers.

385+ r := shellResolverWithPath(t, nil)

386+ m := config.MCPConfig{

387+ Type: config.MCPSSE,

388+ URL: "https://mcp.example.com/events",

389+ Headers: map[string]string{"Authorization": "$MISSING_TOKEN"},

390+ }

391+ tr, err := createTransport(t.Context(), m, r)

392+ require.NoError(t, err)

393+ sse, ok := tr.(*mcp.SSEClientTransport)

394+ require.True(t, ok)

395+ rt, ok := sse.HTTPClient.Transport.(*headerRoundTripper)

396+ require.True(t, ok)

397+ require.NotContains(t, rt.headers, "Authorization")

325398 })

326399}

327400

@@ -356,24 +429,44 @@ func TestCreateSession_ResolutionFailureUpdatesState(t *testing.T) {

356429 wantErrContains: "env FORGEJO_ACCESS_TOKEN",

357430 },

358431 {

432+ // Args that reference an unset bare $VAR no longer

433+ // error out under lenient nounset; the only remaining

434+ // failure mode for arg resolution is a failing $(cmd).

359435 name: "stdio args failure",

360436 mcpName: "test-stdio-args-fail",

361437 cfg: config.MCPConfig{

362438 Type: config.MCPStdio,

363439 Command: "echo",

364− Args: []string{"--token", "$MCP_UNSET_TOKEN"},

440+ Args: []string{"--token", "$(false)"},

365441 },

366442 wantErrContains: "arg 1",

367443 },

368444 {

445+ // Likewise for URL: bare $UNSET expands to ""

446+ // silently, so we need a failing $(cmd) to exercise

447+ // the "url:" wrap from ResolvedURL.

369448 name: "http url failure",

370449 mcpName: "test-http-url-fail",

371450 cfg: config.MCPConfig{

372451 Type: config.MCPHttp,

373− URL: "https://$MCP_MISSING_HOST/api",

452+ URL: "https://$(false)/api",

374453 },

375454 wantErrContains: "url:",

376455 },

456+ {

457+ // A URL whose shell expansion yields the empty

458+ // string (here via ${VAR:-}) is not a ResolvedURL

459+ // error, but the non-empty guard in createTransport

460+ // must still reject it so the state card renders an

461+ // error instead of spawning a transport against "".

462+ name: "http empty-resolved url",

463+ mcpName: "test-http-url-empty",

464+ cfg: config.MCPConfig{

465+ Type: config.MCPHttp,

466+ URL: "${MCP_URL_EMPTY:-}",

467+ },

468+ wantErrContains: "non-empty 'url'",

469+ },

377470 {

378471 name: "http header failure",

379472 mcpName: "test-http-header-fail",

@@ -394,12 +487,17 @@ func TestCreateSession_ResolutionFailureUpdatesState(t *testing.T) {

394487 wantErrContains: "url:",

395488 },

396489 {

490+ // Bare $MISSING in a header resolves to "" silently

491+ // and is then dropped (design decision #18). The

492+ // "header Authorization" wrap only surfaces on a

493+ // $(cmd) failure; that is what this subtest now

494+ // pins for the SSE path.

397495 name: "sse header failure",

398496 mcpName: "test-sse-header-fail",

399497 cfg: config.MCPConfig{

400498 Type: config.MCPSSE,

401499 URL: "https://mcp.example.com/events",

402− Headers: map[string]string{"Authorization": "Bearer $MISSING_SSE_TOKEN"},

500+ Headers: map[string]string{"Authorization": "$(false)"},

403501 },

404502 wantErrContains: "header Authorization",

405503 },

internal/config/config.go+102−3

@@ -108,9 +108,22 @@ type ProviderConfig struct {

108108 // Custom system prompt prefix.

109109 SystemPromptPrefix string `json:"system_prompt_prefix,omitempty" jsonschema:"description=Custom prefix to add to system prompts for this provider"`

110110

111− // Extra headers to send with each request to the provider.

111+ // Extra headers to send with each request to the provider. Values

112+ // run through shell expansion at config-load time, so $VAR and

113+ // $(cmd) work the same way they do in MCP headers. A header whose

114+ // value resolves to the empty string (unset bare $VAR under

115+ // lenient nounset, $(echo), or literal "") is omitted from the

116+ // outgoing request rather than sent as "Header:". See PLAN.md

117+ // Phase 2 design decision #18.

112118 ExtraHeaders map[string]string `json:"extra_headers,omitempty" jsonschema:"description=Additional HTTP headers to send with requests"`

113− // Extra body

119+ // ExtraBody is merged verbatim into OpenAI-compatible request

120+ // bodies. String values are NOT shell-expanded: this is a plain

121+ // JSON passthrough so that arbitrary provider-extension fields

122+ // (numbers, nested objects, booleans) round-trip without a

123+ // recursive walker guessing at intent. If you need an env-var-

124+ // driven value at request time, put it in extra_headers, or in

125+ // the provider's top-level api_key / base_url, all of which do

126+ // expand. See PLAN.md Phase 2 design decision #16.

114127 ExtraBody map[string]any `json:"extra_body,omitempty" jsonschema:"description=Additional fields to include in request bodies, only works with openai-compatible providers"`

115128

116129 ProviderOptions map[string]any `json:"provider_options,omitempty" jsonschema:"description=Additional provider-specific options for this provider"`

@@ -177,7 +190,12 @@ type MCPConfig struct {

177190 DisabledTools []string `json:"disabled_tools,omitempty" jsonschema:"description=List of tools from this MCP server to disable,example=get-library-doc"`

178191 Timeout int `json:"timeout,omitempty" jsonschema:"description=Timeout in seconds for MCP server connections,default=15,example=30,example=60,example=120"`

179192

180− // TODO: maybe make it possible to get the value from the env

193+ // Headers are HTTP headers for HTTP/SSE MCP servers. Values run

194+ // through shell expansion at MCP startup, so $VAR and $(cmd)

195+ // work. A header whose value resolves to the empty string (unset

196+ // bare $VAR under lenient nounset, $(echo), or literal "") is

197+ // omitted from the outgoing request rather than sent as

198+ // "Header:". See PLAN.md Phase 2 design decision #18.

181199 Headers map[string]string `json:"headers,omitempty" jsonschema:"description=HTTP headers for HTTP/SSE MCP servers"`

182200}

183201

@@ -371,6 +389,13 @@ func (m MCPConfig) ResolvedURL(r VariableResolver) (string, error) {

371389// already sanitized by ResolveValue and is wrapped with %w so

372390// errors.Is/As continues to work.

373391//

392+// A header whose value resolves to the empty string (unset bare $VAR

393+// under lenient nounset, $(echo), or literal "") is omitted from the

394+// returned map — sending "X-Auth:" with an empty value is rejected by

395+// some providers and the user's intent in "optional, env-gated

396+// header" is clearly "absent when the var isn't set." See PLAN.md

397+// Phase 2 design decision #18.

398+//

374399// See ResolvedEnv for guidance on picking a resolver.

375400func (m MCPConfig) ResolvedHeaders(r VariableResolver) (map[string]string, error) {

376401 if len(m.Headers) == 0 {

@@ -389,6 +414,80 @@ func (m MCPConfig) ResolvedHeaders(r VariableResolver) (map[string]string, error

389414 if err != nil {

390415 return nil, fmt.Errorf("header %s: %w", k, err)

391416 }

417+ if v == "" {

418+ continue

419+ }

420+ out[k] = v

421+ }

422+ return out, nil

423+}

424+

425+// ResolvedArgs returns l.Args with every element expanded through the

426+// given resolver. A fresh slice is allocated; l.Args is never mutated.

427+// On the first resolution failure it returns nil and an error

428+// identifying the offending positional index; the inner resolver error

429+// is already sanitized by ResolveValue and is wrapped with %w so

430+// errors.Is/As continues to work.

431+//

432+// Empty resolved values are kept (a deliberate "empty positional arg"

433+// like --flag "" is sometimes valid), matching MCPConfig.ResolvedArgs;

434+// see PLAN.md Phase 2 design decision #18.

435+//

436+// The resolver choice matters: in server mode pass the shell resolver

437+// so $VAR / $(cmd) expand; in client mode pass IdentityResolver so the

438+// template is forwarded verbatim. See PLAN.md Phase 2 design decision

439+// #13.

440+func (l LSPConfig) ResolvedArgs(r VariableResolver) ([]string, error) {

441+ if len(l.Args) == 0 {

442+ return nil, nil

443+ }

444+ out := make([]string, len(l.Args))

445+ for i, a := range l.Args {

446+ v, err := r.ResolveValue(a)

447+ if err != nil {

448+ return nil, fmt.Errorf("arg %d: %w", i, err)

449+ }

450+ out[i] = v

451+ }

452+ return out, nil

453+}

454+

455+// ResolvedEnv returns l.Env with every value expanded through the

456+// given resolver. A fresh map is allocated; l.Env is never mutated.

457+// On the first resolution failure it returns nil and an error that

458+// identifies the offending key; the inner resolver error is already

459+// sanitized by ResolveValue and is wrapped with %w so errors.Is/As

460+// continues to work.

461+//

462+// Empty resolved values are kept ("FOO=" is a legitimate request;

463+// opt out via ${VAR:+...}), matching MCPConfig.ResolvedEnv; see

464+// PLAN.md Phase 2 design decision #18.

465+//

466+// Shape note: this returns map[string]string rather than the []string

467+// shape MCPConfig.ResolvedEnv uses because the consumer

468+// (powernap.ClientConfig.Environment in internal/lsp/client.go) takes

469+// a map directly — returning a []string here would only force a

470+// round-trip back to a map at the call site. See PLAN.md Phase 2

471+// design decision #13.

472+//

473+// See ResolvedArgs for guidance on picking a resolver.

474+func (l LSPConfig) ResolvedEnv(r VariableResolver) (map[string]string, error) {

475+ if len(l.Env) == 0 {

476+ return map[string]string{}, nil

477+ }

478+ out := make(map[string]string, len(l.Env))

479+ // Sort keys so failures are reported deterministically when more

480+ // than one value would fail.

481+ keys := make([]string, 0, len(l.Env))

482+ for k := range l.Env {

483+ keys = append(keys, k)

484+ }

485+ slices.Sort(keys)

486+ for _, k := range keys {

487+ v, err := r.ResolveValue(l.Env[k])

488+ if err != nil {

489+ return nil, fmt.Errorf("env %q: %w", k, err)

490+ }

392491 out[k] = v

393492 }

394493 return out, nil

internal/config/load.go+17−8

@@ -222,16 +222,19 @@ func (c *Config) configureProviders(store *ConfigStore, env env.Env, resolver Va

222222 if len(config.ExtraHeaders) > 0 {

223223 maps.Copy(headers, config.ExtraHeaders)

224224 }

225− // Intentional divergence from MCP env/headers/args/url resolution:

226− // provider headers that fail to resolve log and keep their literal

227− // template so providers with optional, env-gated headers still

228− // load on hosts where those vars are unset. Changing this to a

229− // hard error would break those configs. See PLAN.md "Design

230− // decisions" item 4 for the full rationale.

225+ // Provider headers use the same error contract as MCP headers:

226+ // a failing $(...) aborts the provider load with a clear

227+ // message, and a header that resolves to the empty string

228+ // (unset bare $VAR under lenient nounset, $(echo), or literal

229+ // "") is dropped from the outgoing request. See PLAN.md

230+ // Phase 2 design decisions #14 and #18.

231231 for k, v := range headers {

232232 resolved, err := resolver.ResolveValue(v)

233233 if err != nil {

234− slog.Error("Could not resolve provider header", "err", err.Error())

234+ return fmt.Errorf("resolving provider %s header %q: %w", p.ID, k, err)

235+ }

236+ if resolved == "" {

237+ delete(headers, k)

235238 continue

236239 }

237240 headers[k] = resolved

@@ -382,10 +385,16 @@ func (c *Config) configureProviders(store *ConfigStore, env env.Env, resolver Va

382385 continue

383386 }

384387

388+ // Custom-provider headers share the MCP error contract; see

389+ // the known-provider loop above and PLAN.md Phase 2 design

390+ // decisions #14 and #18.

385391 for k, v := range providerConfig.ExtraHeaders {

386392 resolved, err := resolver.ResolveValue(v)

387393 if err != nil {

388− slog.Error("Could not resolve provider header", "err", err.Error())

394+ return fmt.Errorf("resolving provider %s header %q: %w", id, k, err)

395+ }

396+ if resolved == "" {

397+ delete(providerConfig.ExtraHeaders, k)

389398 continue

390399 }

391400 providerConfig.ExtraHeaders[k] = resolved

internal/config/load_test.go+278−64

@@ -77,7 +77,7 @@ func TestConfig_configureProviders(t *testing.T) {

7777 env := env.NewFromMap(map[string]string{

7878 "OPENAI_API_KEY": "test-key",

7979 })

80− resolver := NewEnvironmentVariableResolver(env)

80+ resolver := NewShellVariableResolver(env)

8181 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

8282 require.NoError(t, err)

8383 require.Equal(t, 1, cfg.Providers.Len())

@@ -120,7 +120,7 @@ func TestConfig_configureProvidersWithOverride(t *testing.T) {

120120 env := env.NewFromMap(map[string]string{

121121 "OPENAI_API_KEY": "test-key",

122122 })

123− resolver := NewEnvironmentVariableResolver(env)

123+ resolver := NewShellVariableResolver(env)

124124 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

125125 require.NoError(t, err)

126126 require.Equal(t, 1, cfg.Providers.Len())

@@ -162,7 +162,7 @@ func TestConfig_configureProvidersWithNewProvider(t *testing.T) {

162162 env := env.NewFromMap(map[string]string{

163163 "OPENAI_API_KEY": "test-key",

164164 })

165− resolver := NewEnvironmentVariableResolver(env)

165+ resolver := NewShellVariableResolver(env)

166166 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

167167 require.NoError(t, err)

168168 // Should be to because of the env variable

@@ -198,7 +198,7 @@ func TestConfig_configureProvidersBedrockWithCredentials(t *testing.T) {

198198 "AWS_ACCESS_KEY_ID": "test-key-id",

199199 "AWS_SECRET_ACCESS_KEY": "test-secret-key",

200200 })

201− resolver := NewEnvironmentVariableResolver(env)

201+ resolver := NewShellVariableResolver(env)

202202 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

203203 require.NoError(t, err)

204204 require.Equal(t, cfg.Providers.Len(), 1)

@@ -224,7 +224,7 @@ func TestConfig_configureProvidersBedrockWithoutCredentials(t *testing.T) {

224224 cfg := &Config{}

225225 cfg.setDefaults("/tmp", "")

226226 env := env.NewFromMap(map[string]string{})

227− resolver := NewEnvironmentVariableResolver(env)

227+ resolver := NewShellVariableResolver(env)

228228 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

229229 require.NoError(t, err)

230230 // Provider should not be configured without credentials

@@ -249,7 +249,7 @@ func TestConfig_configureProvidersBedrockWithoutUnsupportedModel(t *testing.T) {

249249 "AWS_ACCESS_KEY_ID": "test-key-id",

250250 "AWS_SECRET_ACCESS_KEY": "test-secret-key",

251251 })

252− resolver := NewEnvironmentVariableResolver(env)

252+ resolver := NewShellVariableResolver(env)

253253 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

254254 require.Error(t, err)

255255}

@@ -272,7 +272,7 @@ func TestConfig_configureProvidersVertexAIWithCredentials(t *testing.T) {

272272 "VERTEXAI_PROJECT": "test-project",

273273 "VERTEXAI_LOCATION": "us-central1",

274274 })

275− resolver := NewEnvironmentVariableResolver(env)

275+ resolver := NewShellVariableResolver(env)

276276 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

277277 require.NoError(t, err)

278278 require.Equal(t, cfg.Providers.Len(), 1)

@@ -304,7 +304,7 @@ func TestConfig_configureProvidersVertexAIWithoutCredentials(t *testing.T) {

304304 "GOOGLE_CLOUD_PROJECT": "test-project",

305305 "GOOGLE_CLOUD_LOCATION": "us-central1",

306306 })

307− resolver := NewEnvironmentVariableResolver(env)

307+ resolver := NewShellVariableResolver(env)

308308 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

309309 require.NoError(t, err)

310310 // Provider should not be configured without proper credentials

@@ -329,7 +329,7 @@ func TestConfig_configureProvidersVertexAIMissingProject(t *testing.T) {

329329 "GOOGLE_GENAI_USE_VERTEXAI": "true",

330330 "GOOGLE_CLOUD_LOCATION": "us-central1",

331331 })

332− resolver := NewEnvironmentVariableResolver(env)

332+ resolver := NewShellVariableResolver(env)

333333 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

334334 require.NoError(t, err)

335335 // Provider should not be configured without project

@@ -353,7 +353,7 @@ func TestConfig_configureProvidersSetProviderID(t *testing.T) {

353353 env := env.NewFromMap(map[string]string{

354354 "OPENAI_API_KEY": "test-key",

355355 })

356− resolver := NewEnvironmentVariableResolver(env)

356+ resolver := NewShellVariableResolver(env)

357357 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

358358 require.NoError(t, err)

359359 require.Equal(t, cfg.Providers.Len(), 1)

@@ -544,7 +544,7 @@ func TestConfig_configureProvidersWithDisabledProvider(t *testing.T) {

544544 env := env.NewFromMap(map[string]string{

545545 "OPENAI_API_KEY": "test-key",

546546 })

547− resolver := NewEnvironmentVariableResolver(env)

547+ resolver := NewShellVariableResolver(env)

548548 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

549549 require.NoError(t, err)

550550

@@ -572,7 +572,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {

572572 cfg.setDefaults("/tmp", "")

573573

574574 env := env.NewFromMap(map[string]string{})

575− resolver := NewEnvironmentVariableResolver(env)

575+ resolver := NewShellVariableResolver(env)

576576 err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})

577577 require.NoError(t, err)

578578

@@ -595,7 +595,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {

595595 cfg.setDefaults("/tmp", "")

596596

597597 env := env.NewFromMap(map[string]string{})

598− resolver := NewEnvironmentVariableResolver(env)

598+ resolver := NewShellVariableResolver(env)

599599 err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})

600600 require.NoError(t, err)

601601

@@ -617,7 +617,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {

617617 cfg.setDefaults("/tmp", "")

618618

619619 env := env.NewFromMap(map[string]string{})

620− resolver := NewEnvironmentVariableResolver(env)

620+ resolver := NewShellVariableResolver(env)

621621 err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})

622622 require.NoError(t, err)

623623

@@ -642,7 +642,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {

642642 cfg.setDefaults("/tmp", "")

643643

644644 env := env.NewFromMap(map[string]string{})

645− resolver := NewEnvironmentVariableResolver(env)

645+ resolver := NewShellVariableResolver(env)

646646 err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})

647647 require.NoError(t, err)

648648

@@ -667,7 +667,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {

667667 cfg.setDefaults("/tmp", "")

668668

669669 env := env.NewFromMap(map[string]string{})

670− resolver := NewEnvironmentVariableResolver(env)

670+ resolver := NewShellVariableResolver(env)

671671 err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})

672672 require.NoError(t, err)

673673

@@ -695,7 +695,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {

695695 cfg.setDefaults("/tmp", "")

696696

697697 env := env.NewFromMap(map[string]string{})

698− resolver := NewEnvironmentVariableResolver(env)

698+ resolver := NewShellVariableResolver(env)

699699 err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})

700700 require.NoError(t, err)

701701

@@ -725,7 +725,7 @@ func TestConfig_configureProvidersCustomProviderValidation(t *testing.T) {

725725 cfg.setDefaults("/tmp", "")

726726

727727 env := env.NewFromMap(map[string]string{})

728− resolver := NewEnvironmentVariableResolver(env)

728+ resolver := NewShellVariableResolver(env)

729729 err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})

730730 require.NoError(t, err)

731731

@@ -760,7 +760,7 @@ func TestConfig_configureProvidersEnhancedCredentialValidation(t *testing.T) {

760760 env := env.NewFromMap(map[string]string{

761761 "GOOGLE_GENAI_USE_VERTEXAI": "false",

762762 })

763− resolver := NewEnvironmentVariableResolver(env)

763+ resolver := NewShellVariableResolver(env)

764764 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

765765 require.NoError(t, err)

766766

@@ -791,7 +791,7 @@ func TestConfig_configureProvidersEnhancedCredentialValidation(t *testing.T) {

791791 cfg.setDefaults("/tmp", "")

792792

793793 env := env.NewFromMap(map[string]string{})

794− resolver := NewEnvironmentVariableResolver(env)

794+ resolver := NewShellVariableResolver(env)

795795 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

796796 require.NoError(t, err)

797797

@@ -822,7 +822,7 @@ func TestConfig_configureProvidersEnhancedCredentialValidation(t *testing.T) {

822822 cfg.setDefaults("/tmp", "")

823823

824824 env := env.NewFromMap(map[string]string{})

825− resolver := NewEnvironmentVariableResolver(env)

825+ resolver := NewShellVariableResolver(env)

826826 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

827827 require.NoError(t, err)

828828

@@ -855,7 +855,7 @@ func TestConfig_configureProvidersEnhancedCredentialValidation(t *testing.T) {

855855 env := env.NewFromMap(map[string]string{

856856 "OPENAI_API_KEY": "test-key",

857857 })

858− resolver := NewEnvironmentVariableResolver(env)

858+ resolver := NewShellVariableResolver(env)

859859 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

860860 require.NoError(t, err)

861861

@@ -889,7 +889,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {

889889 cfg := &Config{}

890890 cfg.setDefaults("/tmp", "")

891891 env := env.NewFromMap(map[string]string{})

892− resolver := NewEnvironmentVariableResolver(env)

892+ resolver := NewShellVariableResolver(env)

893893 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

894894 require.NoError(t, err)

895895

@@ -925,7 +925,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {

925925 cfg := &Config{}

926926 cfg.setDefaults("/tmp", "")

927927 env := env.NewFromMap(map[string]string{})

928− resolver := NewEnvironmentVariableResolver(env)

928+ resolver := NewShellVariableResolver(env)

929929 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

930930 require.NoError(t, err)

931931

@@ -955,7 +955,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {

955955 cfg := &Config{}

956956 cfg.setDefaults("/tmp", "")

957957 env := env.NewFromMap(map[string]string{})

958− resolver := NewEnvironmentVariableResolver(env)

958+ resolver := NewShellVariableResolver(env)

959959 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

960960 require.NoError(t, err)

961961 _, _, err = cfg.defaultModelSelection(knownProviders)

@@ -998,7 +998,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {

998998 }

999999 cfg.setDefaults("/tmp", "")

10001000 env := env.NewFromMap(map[string]string{})

1001− resolver := NewEnvironmentVariableResolver(env)

1001+ resolver := NewShellVariableResolver(env)

10021002 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

10031003 require.NoError(t, err)

10041004 large, small, err := cfg.defaultModelSelection(knownProviders)

@@ -1042,7 +1042,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {

10421042 }

10431043 cfg.setDefaults("/tmp", "")

10441044 env := env.NewFromMap(map[string]string{})

1045− resolver := NewEnvironmentVariableResolver(env)

1045+ resolver := NewShellVariableResolver(env)

10461046 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

10471047 require.NoError(t, err)

10481048 _, _, err = cfg.defaultModelSelection(knownProviders)

@@ -1084,7 +1084,7 @@ func TestConfig_defaultModelSelection(t *testing.T) {

10841084 }

10851085 cfg.setDefaults("/tmp", "")

10861086 env := env.NewFromMap(map[string]string{})

1087− resolver := NewEnvironmentVariableResolver(env)

1087+ resolver := NewShellVariableResolver(env)

10881088 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

10891089 require.NoError(t, err)

10901090 large, small, err := cfg.defaultModelSelection(knownProviders)

@@ -1129,7 +1129,7 @@ func TestConfig_configureProvidersDisableDefaultProviders(t *testing.T) {

11291129 env := env.NewFromMap(map[string]string{

11301130 "OPENAI_API_KEY": "test-key",

11311131 })

1132− resolver := NewEnvironmentVariableResolver(env)

1132+ resolver := NewShellVariableResolver(env)

11331133 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

11341134 require.ErrorContains(t, err, "no custom providers")

11351135

@@ -1172,7 +1172,7 @@ func TestConfig_configureProvidersDisableDefaultProviders(t *testing.T) {

11721172 "MY_API_KEY": "test-key",

11731173 "OPENAI_API_KEY": "test-key",

11741174 })

1175− resolver := NewEnvironmentVariableResolver(env)

1175+ resolver := NewShellVariableResolver(env)

11761176 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

11771177 require.NoError(t, err)

11781178

@@ -1226,7 +1226,7 @@ func TestConfig_configureProvidersDisableDefaultProviders(t *testing.T) {

12261226 "OPENAI_API_KEY": "test-key",

12271227 "ANTHROPIC_API_KEY": "test-key",

12281228 })

1229− resolver := NewEnvironmentVariableResolver(env)

1229+ resolver := NewShellVariableResolver(env)

12301230 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

12311231 require.NoError(t, err)

12321232

@@ -1254,7 +1254,7 @@ func TestConfig_configureProvidersDisableDefaultProviders(t *testing.T) {

12541254 cfg.setDefaults("/tmp", "")

12551255

12561256 env := env.NewFromMap(map[string]string{})

1257− resolver := NewEnvironmentVariableResolver(env)

1257+ resolver := NewShellVariableResolver(env)

12581258 err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})

12591259 require.ErrorContains(t, err, "no custom providers")

12601260

@@ -1278,7 +1278,7 @@ func TestConfig_configureProvidersDisableDefaultProviders(t *testing.T) {

12781278 cfg.setDefaults("/tmp", "")

12791279

12801280 env := env.NewFromMap(map[string]string{})

1281− resolver := NewEnvironmentVariableResolver(env)

1281+ resolver := NewShellVariableResolver(env)

12821282 err := cfg.configureProviders(testStore(cfg), env, resolver, []catwalk.Provider{})

12831283 require.ErrorContains(t, err, "no custom providers")

12841284

@@ -1336,7 +1336,7 @@ func TestConfig_configureSelectedModels(t *testing.T) {

13361336 cfg.setDefaults(dir, "")

13371337 store := &ConfigStore{config: cfg, globalDataPath: globalPath}

13381338 env := env.NewFromMap(map[string]string{})

1339− resolver := NewEnvironmentVariableResolver(env)

1339+ resolver := NewShellVariableResolver(env)

13401340 err := cfg.configureProviders(store, env, resolver, knownProviders)

13411341 require.NoError(t, err)

13421342

@@ -1386,7 +1386,7 @@ func TestConfig_configureSelectedModels(t *testing.T) {

13861386 }

13871387 cfg.setDefaults("/tmp", "")

13881388 env := env.NewFromMap(map[string]string{})

1389− resolver := NewEnvironmentVariableResolver(env)

1389+ resolver := NewShellVariableResolver(env)

13901390 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

13911391 require.NoError(t, err)

13921392

@@ -1448,7 +1448,7 @@ func TestConfig_configureSelectedModels(t *testing.T) {

14481448 }

14491449 cfg.setDefaults("/tmp", "")

14501450 env := env.NewFromMap(map[string]string{})

1451− resolver := NewEnvironmentVariableResolver(env)

1451+ resolver := NewShellVariableResolver(env)

14521452 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

14531453 require.NoError(t, err)

14541454

@@ -1493,7 +1493,7 @@ func TestConfig_configureSelectedModels(t *testing.T) {

14931493 }

14941494 cfg.setDefaults("/tmp", "")

14951495 env := env.NewFromMap(map[string]string{})

1496− resolver := NewEnvironmentVariableResolver(env)

1496+ resolver := NewShellVariableResolver(env)

14971497 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

14981498 require.NoError(t, err)

14991499

@@ -1532,7 +1532,7 @@ func TestConfig_configureProviders_HyperAPIKeyFromEnv(t *testing.T) {

15321532 env := env.NewFromMap(map[string]string{

15331533 "HYPER_API_KEY": "env-api-key",

15341534 })

1535− resolver := NewEnvironmentVariableResolver(env)

1535+ resolver := NewShellVariableResolver(env)

15361536 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

15371537 require.NoError(t, err)

15381538 require.Equal(t, 1, cfg.Providers.Len())

@@ -1579,7 +1579,7 @@ func TestConfig_configureProviders_HyperAPIKeyFromConfigOverrides(t *testing.T)

15791579 env := env.NewFromMap(map[string]string{

15801580 "HYPER_API_KEY": "env-api-key",

15811581 })

1582− resolver := NewEnvironmentVariableResolver(env)

1582+ resolver := NewShellVariableResolver(env)

15831583 err := cfg.configureProviders(testStore(cfg), env, resolver, knownProviders)

15841584 require.NoError(t, err)

15851585 require.Equal(t, 1, cfg.Providers.Len())

@@ -1590,18 +1590,13 @@ func TestConfig_configureProviders_HyperAPIKeyFromConfigOverrides(t *testing.T)

15901590 require.Equal(t, "env-api-key", pc.APIKey)

15911591}

15921592

1593−// TestConfig_configureProviders_ProviderHeaderResolveFailure pins the

1594−// intentional divergence at load.go:225: a provider header whose

1595−// expansion fails must NOT fail the whole config load. It logs an

1596−// error, keeps the literal template in the resolved header map, and

1597−// moves on. The MCP contract (hard error on failed expansion) does not

1598−// apply here because many providers ship DefaultHeaders that reference

1599−// env vars which are legitimately unset on most hosts.

1600−//

1601−// If this test ever flips to requiring an error, read PLAN.md "Design

1602−// decisions" item 4 before changing the production code — the

1603−// divergence is deliberate.

1604−func TestConfig_configureProviders_ProviderHeaderResolveFailure(t *testing.T) {

1593+// TestConfig_configureProviders_ProviderHeaderResolveError pins

1594+// Phase 2 design decision #14: a failing $(cmd) in a provider header

1595+// must fail the provider load with a clear message that names the

1596+// offending header. The Phase 1 log-and-continue divergence at

1597+// load.go:225 is gone; provider headers now share the MCP error

1598+// contract.

1599+func TestConfig_configureProviders_ProviderHeaderResolveError(t *testing.T) {

16051600 knownProviders := []catwalk.Provider{

16061601 {

16071602 ID: "openai",

@@ -1615,13 +1610,9 @@ func TestConfig_configureProviders_ProviderHeaderResolveFailure(t *testing.T) {

16151610 Providers: csync.NewMapFrom(map[string]ProviderConfig{

16161611 "openai": {

16171612 ExtraHeaders: map[string]string{

1618− // Failing $(...) — inner command exits 1, no stdout.

1613+ // Failing $(...) — inner command exits 1. Must

1614+ // propagate as an error, not a silent truncation.

16191615 "X-Broken": "$(false)",

1620− // Unset var — nounset makes this an error too.

1621− "X-Missing": "$PROVIDER_HEADER_NEVER_SET",

1622− // Happy path: must still be resolved, proving the

1623− // failure in the other two did not abort the loop.

1624− "X-Static": "kept-literal",

16251616 },

16261617 },

16271618 }),

@@ -1635,15 +1626,238 @@ func TestConfig_configureProviders_ProviderHeaderResolveFailure(t *testing.T) {

16351626 resolver := NewShellVariableResolver(testEnv)

16361627

16371628 err := cfg.configureProviders(testStore(cfg), testEnv, resolver, knownProviders)

1638− require.NoError(t, err, "provider load must tolerate failing header expansion")

1629+ require.Error(t, err, "failing $(cmd) in a header must fail the provider load")

1630+ require.Contains(t, err.Error(), "X-Broken", "error must name the offending header")

1631+}

1632+

1633+// TestConfig_configureProviders_CatwalkDefaultWithUnsetVarLoads pins

1634+// Phase 2 design decisions #11 and #18 from the provider angle: a

1635+// Catwalk-style default header like

1636+// "OpenAI-Organization": "$OPENAI_ORG_ID" must load cleanly under

1637+// lenient nounset (unset → "" → header dropped), not fail the load

1638+// and not leave the literal template on the wire.

1639+func TestConfig_configureProviders_CatwalkDefaultWithUnsetVarLoads(t *testing.T) {

1640+ knownProviders := []catwalk.Provider{

1641+ {

1642+ ID: "openai",

1643+ APIKey: "$OPENAI_API_KEY",

1644+ APIEndpoint: "https://api.openai.com/v1",

1645+ Models: []catwalk.Model{{ID: "test-model"}},

1646+ DefaultHeaders: map[string]string{

1647+ "OpenAI-Organization": "$OPENAI_ORG_ID",

1648+ },

1649+ },

1650+ }

1651+

1652+ cfg := &Config{}

1653+ cfg.setDefaults("/tmp", "")

1654+

1655+ testEnv := env.NewFromMap(map[string]string{

1656+ "OPENAI_API_KEY": "test-key",

1657+ "PATH": os.Getenv("PATH"),

1658+ })

1659+ resolver := NewShellVariableResolver(testEnv)

1660+

1661+ err := cfg.configureProviders(testStore(cfg), testEnv, resolver, knownProviders)

1662+ require.NoError(t, err, "optional env-gated header must not fail the load")

16391663

16401664 pc, ok := cfg.Providers.Get("openai")

16411665 require.True(t, ok, "openai provider must still be configured")

1666+ _, present := pc.ExtraHeaders["OpenAI-Organization"]

1667+ require.False(t, present, "header whose value resolves to empty must be absent")

1668+}

1669+

1670+// TestConfig_configureProviders_LiteralEmptyHeaderDropped pins design

1671+// decision #18 for the literal case: a user-authored

1672+// "X-Custom": "" in extra_headers is absent from the resolved map.

1673+// Applies to both known- and custom-provider paths; this test

1674+// exercises the custom-provider loop.

1675+func TestConfig_configureProviders_LiteralEmptyHeaderDropped(t *testing.T) {

1676+ cfg := &Config{

1677+ Providers: csync.NewMapFrom(map[string]ProviderConfig{

1678+ "my-llm": {

1679+ APIKey: "test-key",

1680+ BaseURL: "https://my-llm.example.com/v1",

1681+ Type: catwalk.TypeOpenAI,

1682+ Models: []catwalk.Model{{ID: "m"}},

1683+ ExtraHeaders: map[string]string{

1684+ "X-Custom": "",

1685+ "X-Kept": "present",

1686+ },

1687+ },

1688+ }),

1689+ }

1690+ cfg.setDefaults("/tmp", "")

1691+

1692+ testEnv := env.NewFromMap(map[string]string{

1693+ "PATH": os.Getenv("PATH"),

1694+ })

1695+ resolver := NewShellVariableResolver(testEnv)

1696+

1697+ err := cfg.configureProviders(testStore(cfg), testEnv, resolver, []catwalk.Provider{})

1698+ require.NoError(t, err)

1699+

1700+ pc, ok := cfg.Providers.Get("my-llm")

1701+ require.True(t, ok)

1702+ _, present := pc.ExtraHeaders["X-Custom"]

1703+ require.False(t, present, "literal empty-string header must be dropped")

1704+ require.Equal(t, "present", pc.ExtraHeaders["X-Kept"])

1705+}

1706+

1707+// TestConfig_configureProviders_EchoEmptyHeaderDropped pins design

1708+// decision #18 for the non-failing empty case: $(echo) exits 0 with

1709+// empty output, resolves cleanly to "", and must be dropped the same

1710+// way an unset bare $VAR is. Exercises the known-provider loop.

1711+func TestConfig_configureProviders_EchoEmptyHeaderDropped(t *testing.T) {

1712+ knownProviders := []catwalk.Provider{

1713+ {

1714+ ID: "openai",

1715+ APIKey: "$OPENAI_API_KEY",

1716+ APIEndpoint: "https://api.openai.com/v1",

1717+ Models: []catwalk.Model{{ID: "test-model"}},

1718+ DefaultHeaders: map[string]string{

1719+ "X-Empty": "$(echo)",

1720+ "X-Kept": "present",

1721+ },

1722+ },

1723+ }

1724+

1725+ cfg := &Config{}

1726+ cfg.setDefaults("/tmp", "")

1727+

1728+ testEnv := env.NewFromMap(map[string]string{

1729+ "OPENAI_API_KEY": "test-key",

1730+ "PATH": os.Getenv("PATH"),

1731+ })

1732+ resolver := NewShellVariableResolver(testEnv)

1733+

1734+ err := cfg.configureProviders(testStore(cfg), testEnv, resolver, knownProviders)

1735+ require.NoError(t, err)

1736+

1737+ pc, ok := cfg.Providers.Get("openai")

1738+ require.True(t, ok)

1739+ _, present := pc.ExtraHeaders["X-Empty"]

1740+ require.False(t, present, "$(echo) → empty → header must be dropped")

1741+ require.Equal(t, "present", pc.ExtraHeaders["X-Kept"])

1742+}

1743+

1744+// TestConfig_configureProviders_UnsetAPIKeySkipsProvider pins Phase 2

1745+// Step 12 / design decision #15: under the lenient-nounset shell

1746+// resolver, $UNSET_API_KEY expands to ("", nil) rather than ("", err),

1747+// and the existing `v == "" || err != nil` skip path at load.go:331

1748+// still drops the provider. The slog.Warn line is emitted on the same

1749+// path but is not asserted here — internal/config/load_test.go's

1750+// TestMain replaces the default slog handler with an io.Discard

1751+// writer, so capturing that log line would require mid-test handler

1752+// swapping and a sync.Mutex dance that adds more flake surface than

1753+// signal. The observable outcome (provider absent from the map) is

1754+// what downstream code — model picker, agent wiring — actually reads,

1755+// so that's what we pin.

1756+func TestConfig_configureProviders_UnsetAPIKeySkipsProvider(t *testing.T) {

1757+ knownProviders := []catwalk.Provider{

1758+ {

1759+ ID: "openai",

1760+ APIKey: "$SOMETHING_UNSET",

1761+ APIEndpoint: "https://api.openai.com/v1",

1762+ Models: []catwalk.Model{{ID: "test-model"}},

1763+ },

1764+ }

1765+

1766+ // Existing user config for this known provider so the load.go:332

1767+ // `if configExists` branch fires and actually calls Providers.Del.

1768+ // Without it the provider was never in the map to begin with and

1769+ // the test would pass trivially.

1770+ cfg := &Config{

1771+ Providers: csync.NewMapFrom(map[string]ProviderConfig{

1772+ "openai": {BaseURL: "custom-url"},

1773+ }),

1774+ }

1775+ cfg.setDefaults("/tmp", "")

1776+

1777+ testEnv := env.NewFromMap(map[string]string{

1778+ "PATH": os.Getenv("PATH"),

1779+ })

1780+ resolver := NewShellVariableResolver(testEnv)

1781+

1782+ err := cfg.configureProviders(testStore(cfg), testEnv, resolver, knownProviders)

1783+ require.NoError(t, err, "skip path must not surface as a load error")

1784+

1785+ require.Equal(t, 0, cfg.Providers.Len(), "provider with unset API key must be skipped")

1786+ _, exists := cfg.Providers.Get("openai")

1787+ require.False(t, exists)

1788+}

1789+

1790+// TestConfig_configureProviders_FailingAPIKeyCmdSkipsProvider pins

1791+// that the two failure modes for APIKey — ("", nil) from an unset var

1792+// under lenient nounset and ("", err) from a failing $(cmd) — are

1793+// equivalent for the skip outcome at load.go:331. The `v == "" ||

1794+// err != nil` check fires on either branch; this test locks in that

1795+// equivalence so a future refactor that splits the check into two

1796+// paths doesn't accidentally start propagating $(false) as a load

1797+// error while keeping unset-var as a silent skip (or vice versa).

1798+func TestConfig_configureProviders_FailingAPIKeyCmdSkipsProvider(t *testing.T) {

1799+ knownProviders := []catwalk.Provider{

1800+ {

1801+ ID: "openai",

1802+ APIKey: "$(false)",

1803+ APIEndpoint: "https://api.openai.com/v1",

1804+ Models: []catwalk.Model{{ID: "test-model"}},

1805+ },

1806+ }

1807+

1808+ cfg := &Config{

1809+ Providers: csync.NewMapFrom(map[string]ProviderConfig{

1810+ "openai": {BaseURL: "custom-url"},

1811+ }),

1812+ }

1813+ cfg.setDefaults("/tmp", "")

1814+

1815+ testEnv := env.NewFromMap(map[string]string{

1816+ "PATH": os.Getenv("PATH"),

1817+ })

1818+ resolver := NewShellVariableResolver(testEnv)

1819+

1820+ err := cfg.configureProviders(testStore(cfg), testEnv, resolver, knownProviders)

1821+ require.NoError(t, err, "failing $(cmd) in API key must skip provider, not fail load")

1822+

1823+ require.Equal(t, 0, cfg.Providers.Len(), "provider with failing $(cmd) API key must be skipped")

1824+ _, exists := cfg.Providers.Get("openai")

1825+ require.False(t, exists)

1826+}

1827+

1828+// TestConfig_configureProviders_UnsetAzureEndpointSkipsProvider pins

1829+// the same contract on the Azure path at load.go:287 — APIEndpoint is

1830+// the field that gates Azure and goes through the same

1831+// `v == "" || err != nil` skip check. Covered here so both branches

1832+// of the shared skip pattern (APIKey default path and APIEndpoint

1833+// Azure path) are tested; a future refactor that unifies them can

1834+// rely on these two tests to catch drift.

1835+func TestConfig_configureProviders_UnsetAzureEndpointSkipsProvider(t *testing.T) {

1836+ knownProviders := []catwalk.Provider{

1837+ {

1838+ ID: catwalk.InferenceProviderAzure,

1839+ APIKey: "test-key",

1840+ APIEndpoint: "$UNSET_AZURE_ENDPOINT",

1841+ Models: []catwalk.Model{{ID: "test-model"}},

1842+ },

1843+ }

1844+

1845+ cfg := &Config{

1846+ Providers: csync.NewMapFrom(map[string]ProviderConfig{

1847+ "azure": {BaseURL: ""},

1848+ }),

1849+ }

1850+ cfg.setDefaults("/tmp", "")

1851+

1852+ testEnv := env.NewFromMap(map[string]string{

1853+ "PATH": os.Getenv("PATH"),

1854+ })

1855+ resolver := NewShellVariableResolver(testEnv)

1856+

1857+ err := cfg.configureProviders(testStore(cfg), testEnv, resolver, knownProviders)

1858+ require.NoError(t, err)

16421859

1643− // Literal template preserved for the two failing headers; the

1644− // happy-path header is resolved through the shell (pass-through

1645− // for a literal value).

1646− require.Equal(t, "$(false)", pc.ExtraHeaders["X-Broken"])

1647− require.Equal(t, "$PROVIDER_HEADER_NEVER_SET", pc.ExtraHeaders["X-Missing"])

1648− require.Equal(t, "kept-literal", pc.ExtraHeaders["X-Static"])

1860+ require.Equal(t, 0, cfg.Providers.Len(), "azure provider with unset endpoint must be skipped")

1861+ _, exists := cfg.Providers.Get("azure")

1862+ require.False(t, exists)

16491863}

internal/config/mcp_resolved_url_test.go+20−3

@@ -44,14 +44,31 @@ func TestMCPConfig_ResolvedURL(t *testing.T) {

4444 require.Equal(t, "https://mcp.example.com/events", got)

4545 })

4646

47− t.Run("unset var is an error wrapping the template", func(t *testing.T) {

47+ t.Run("unset var expands to empty under lenient default", func(t *testing.T) {

4848 t.Parallel()

49+ // Phase 2 defaults to nounset-off: bare $VAR on an unset

50+ // variable expands to "" rather than erroring. Here the

51+ // host collapses to empty, so the caller sees a malformed

52+ // URL rather than a resolver error; that's the expected

53+ // trade-off for making $OPTIONAL-style patterns work, and

54+ // required-credential callers should use ${VAR:?msg}.

4955 m := MCPConfig{Type: MCPHttp, URL: "https://$MCP_MISSING_HOST/api"}

56+ got, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil)))

57+ require.NoError(t, err, "unset var must not error under lenient default")

58+ require.Equal(t, "https:///api", got)

59+ })

60+

61+ t.Run("colon-question on unset var errors regardless of toggle", func(t *testing.T) {

62+ t.Parallel()

63+ // ${VAR:?msg} is the opt-in strictness mechanism; it must

64+ // hard-error even with NoUnset off so required credentials

65+ // surface at load time instead of shipping empty-host URLs

66+ // to the transport layer.

67+ m := MCPConfig{Type: MCPHttp, URL: "https://${MCP_MISSING_HOST:?set MCP_MISSING_HOST}/api"}

5068 _, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil)))

5169 require.Error(t, err)

5270 require.Contains(t, err.Error(), "url:")

53− require.Contains(t, err.Error(), "$MCP_MISSING_HOST")

54− require.Contains(t, err.Error(), "unbound")

71+ require.Contains(t, err.Error(), "set MCP_MISSING_HOST")

5572 })

5673

5774 t.Run("failing command substitution is an error", func(t *testing.T) {

internal/config/resolve.go+12−31

@@ -10,8 +10,7 @@ import (

1010)

1111

1212// resolveTimeout bounds how long a single ResolveValue call may spend

13−// inside shell expansion (including any command substitution). Matches

14−// the timeout used by the previous hand-rolled parser.

13+// inside shell expansion (including any command substitution).

1514const resolveTimeout = 5 * time.Minute

1615

1716type VariableResolver interface {

@@ -58,8 +57,11 @@ type shellVariableResolver struct {

5857// NewShellVariableResolver returns a VariableResolver that delegates to

5958// the embedded shell (the same interpreter used by the bash tool and

6059// hooks). Supported constructs match shell.ExpandValue: $VAR, ${VAR},

61−// ${VAR:-default}, $(command), quoting, and escapes. Unset variables are

62−// an error; use ${VAR:-} to opt in to an empty fallback.

60+// ${VAR:-default}, $(command), quoting, and escapes. Unset variables

61+// expand to the empty string by default, matching bash; use

62+// ${VAR:?message} to require a value and fail loudly when it is missing.

63+// The stricter "unset is always an error" mode is gated globally by

64+// shell.NoUnset.

6365func NewShellVariableResolver(e env.Env, opts ...ShellResolverOption) VariableResolver {

6466 r := &shellVariableResolver{

6567 env: e,

@@ -77,9 +79,12 @@ func NewShellVariableResolver(e env.Env, opts ...ShellResolverOption) VariableRe

7779// - $VAR and ${VAR} for environment variables.

7880// - ${VAR:-default} / ${VAR:+alt} / ${VAR:?msg} for defaulting.

7981//

80−// Unset variables are a hard error (nounset), mirroring the historical

81−// behaviour of this resolver: silently expanding an unset variable to the

82−// empty string is exactly how broken credentials reach MCP servers.

82+// Unset variables expand to the empty string by default, matching bash.

83+// Command-substitution failures are always a hard error. Required

84+// credentials should use ${VAR:?message} so a missing variable fails

85+// loudly at load time instead of quietly resolving to empty. Global

86+// strict mode is available via shell.NoUnset for callers that want the

87+// old nounset-on behaviour back.

8388func (r *shellVariableResolver) ResolveValue(value string) (string, error) {

8489 // Preserve the historical backward-compat contract: a lone "$" is a

8590 // malformed config value, not a legal literal. The underlying shell

@@ -167,27 +172,3 @@ func scrubErrorMessage(s string) string {

167172 }

168173 return string(out)

169174}

170−

171−type environmentVariableResolver struct {

172− env env.Env

173−}

174−

175−func NewEnvironmentVariableResolver(env env.Env) VariableResolver {

176− return &environmentVariableResolver{

177− env: env,

178− }

179−}

180−

181−// ResolveValue resolves environment variables from the provided env.Env.

182−func (r *environmentVariableResolver) ResolveValue(value string) (string, error) {

183− if len(value) == 0 || value[0] != '$' {

184− return value, nil

185− }

186−

187− varName := value[1:]

188− resolvedValue := r.env.Get(varName)

189− if resolvedValue == "" {

190− return "", fmt.Errorf("environment variable %q not set", varName)

191− }

192− return resolvedValue, nil

193−}

internal/config/resolve_real_test.go+214−10

@@ -159,23 +159,50 @@ func TestResolvedEnv_RealShell_Deterministic(t *testing.T) {

159159 require.True(t, slices.IsSorted(got), "env slice must be sorted; got %v", got)

160160}

161161

162−// TestResolvedEnv_RealShell_NounsetRegression is the single most

163−// important assertion in this file: an unset variable is an error, not

164−// an empty expansion. Swapping the hand-rolled parser for mvdan's

165−// default-expansion behaviour without nounset would re-introduce

166−// Defect 1 via a typo'd variable name.

167−func TestResolvedEnv_RealShell_NounsetRegression(t *testing.T) {

162+// TestResolvedEnv_RealShell_UnsetExpandsEmpty pins Phase 2's lenient

163+// default: an unset bare $VAR expands to the empty string, matching

164+// bash. The silent-empty-credential class of bug that motivated Phase

165+// 1's nounset-on default is already prevented by the pure-function

166+// error-returning contract of ResolvedEnv, so we no longer rely on

167+// nounset to catch typo'd variable names. Users who want strict

168+// behaviour for a required credential opt in per-reference with

169+// ${VAR:?msg}; see TestResolvedEnv_RealShell_ColonQuestionIsStrict.

170+func TestResolvedEnv_RealShell_UnsetExpandsEmpty(t *testing.T) {

168171 t.Parallel()

169172

170173 m := MCPConfig{Env: map[string]string{

171− // Intentional typo: user meant $FORGEJO_TOKEN.

174+ // Intentional typo: user meant $FORGEJO_TOKEN. Under Phase 2

175+ // defaults this expands to "" rather than erroring, matching

176+ // bash's behaviour on bare $VAR.

172177 "FORGEJO_ACCESS_TOKEN": "$FORGJO_TOKEN",

173178 }}

174179 got, err := m.ResolvedEnv(realShellResolver(nil))

175− require.Error(t, err, "unset var must not silently expand to empty")

180+ require.NoError(t, err, "unset var must expand to empty, not error")

181+ require.Equal(t, []string{"FORGEJO_ACCESS_TOKEN="}, got)

182+}

183+

184+// TestResolvedEnv_RealShell_ColonQuestionIsStrict pins the opt-in

185+// strictness contract: ${VAR:?msg} must hard-error when VAR is unset,

186+// regardless of the global NoUnset toggle. This is the recommended

187+// mechanism for required credentials under the lenient default, so a

188+// future refactor that accidentally swallows ${VAR:?...} errors would

189+// silently ship empty tokens to MCP servers again.

190+func TestResolvedEnv_RealShell_ColonQuestionIsStrict(t *testing.T) {

191+ t.Parallel()

192+

193+ m := MCPConfig{Env: map[string]string{

194+ "FORGEJO_ACCESS_TOKEN": "${FORGJO_TOKEN:?set FORGJO_TOKEN}",

195+ }}

196+ got, err := m.ResolvedEnv(realShellResolver(nil))

197+ require.Error(t, err, "${VAR:?msg} must error when VAR is unset")

176198 require.Nil(t, got)

177− require.Contains(t, err.Error(), "FORGEJO_ACCESS_TOKEN")

178− require.Contains(t, err.Error(), "$FORGJO_TOKEN")

199+ // The resolver wraps with the env key and the user-written

200+ // template; the inner shell error carries the :? message so

201+ // users learn which credential is missing and why.

202+ msg := err.Error()

203+ require.Contains(t, msg, "FORGEJO_ACCESS_TOKEN")

204+ require.Contains(t, msg, "${FORGJO_TOKEN:?set FORGJO_TOKEN}")

205+ require.Contains(t, msg, "set FORGJO_TOKEN")

179206}

180207

181208// TestResolvedEnv_RealShell_FailureDetail pins that a failing inner

@@ -232,6 +259,71 @@ func TestResolvedHeaders_RealShell_FailurePreservesOriginal(t *testing.T) {

232259 require.Equal(t, orig, m.Headers, "receiver Headers must be preserved")

233260}

234261

262+// TestResolvedHeaders_RealShell_DropEmpty pins Phase 2 design

263+// decision #18 on the MCP side: a header whose value resolves to the

264+// empty string is omitted from the returned map. Covers the three

265+// ways a value can legitimately land on empty — unset bare $VAR

266+// under lenient nounset, a literal "", and a non-failing command

267+// whose stdout is empty — and also pins that a failing $(cmd) still

268+// errors rather than silently dropping.

269+func TestResolvedHeaders_RealShell_DropEmpty(t *testing.T) {

270+ t.Parallel()

271+

272+ t.Run("unset $VAR is absent", func(t *testing.T) {

273+ t.Parallel()

274+ m := MCPConfig{Headers: map[string]string{

275+ "X-Missing": "$MCP_HEADER_NEVER_SET",

276+ "X-Kept": "present",

277+ }}

278+ got, err := m.ResolvedHeaders(realShellResolver(nil))

279+ require.NoError(t, err)

280+ _, present := got["X-Missing"]

281+ require.False(t, present, "unset bare $VAR → empty → header dropped")

282+ require.Equal(t, "present", got["X-Kept"])

283+ })

284+

285+ t.Run("literal empty string is absent", func(t *testing.T) {

286+ t.Parallel()

287+ m := MCPConfig{Headers: map[string]string{

288+ "X-Custom": "",

289+ "X-Kept": "present",

290+ }}

291+ got, err := m.ResolvedHeaders(realShellResolver(nil))

292+ require.NoError(t, err)

293+ _, present := got["X-Custom"]

294+ require.False(t, present, "literal empty-string header must be dropped")

295+ require.Equal(t, "present", got["X-Kept"])

296+ })

297+

298+ t.Run("$(echo) is absent", func(t *testing.T) {

299+ t.Parallel()

300+ m := MCPConfig{Headers: map[string]string{

301+ "X-Empty": "$(echo)",

302+ "X-Kept": "present",

303+ }}

304+ got, err := m.ResolvedHeaders(realShellResolver(nil))

305+ require.NoError(t, err)

306+ _, present := got["X-Empty"]

307+ require.False(t, present, "$(echo) → empty → header dropped")

308+ require.Equal(t, "present", got["X-Kept"])

309+ })

310+

311+ t.Run("$(false) errors and does not mutate", func(t *testing.T) {

312+ t.Parallel()

313+ m := MCPConfig{Headers: map[string]string{

314+ "X-Broken": "$(false)",

315+ "X-Kept": "present",

316+ }}

317+ orig := maps.Clone(m.Headers)

318+

319+ got, err := m.ResolvedHeaders(realShellResolver(nil))

320+ require.Error(t, err)

321+ require.Empty(t, got, "map must be nil/empty on failure, not a partial")

322+ require.Contains(t, err.Error(), "header X-Broken")

323+ require.Equal(t, orig, m.Headers, "receiver Headers must be preserved")

324+ })

325+}

326+

235327// TestResolvedArgs_RealShell exercises both success and failure for

236328// m.Args symmetrically with Env. Args are ordered so error messages

237329// must identify a positional index, not a key.

@@ -267,6 +359,118 @@ func TestResolvedArgs_RealShell(t *testing.T) {

267359 })

268360}

269361

362+// TestLSPConfig_ResolvedArgs_RealShell exercises both success and

363+// failure for l.Args symmetrically with MCP args. Args are ordered so

364+// error messages must identify a positional index, not a key.

365+func TestLSPConfig_ResolvedArgs_RealShell(t *testing.T) {

366+ t.Parallel()

367+

368+ t.Run("success expands $VAR in each element", func(t *testing.T) {

369+ t.Parallel()

370+ l := LSPConfig{Args: []string{"--root", "$HOME", "--flag", "literal"}}

371+ r := realShellResolver(map[string]string{"HOME": "/home/tester"})

372+ got, err := l.ResolvedArgs(r)

373+ require.NoError(t, err)

374+ require.Equal(t, []string{"--root", "/home/tester", "--flag", "literal"}, got)

375+ })

376+

377+ t.Run("failure identifies offending index", func(t *testing.T) {

378+ t.Parallel()

379+ l := LSPConfig{Args: []string{"--root", "$(false)"}}

380+ orig := slices.Clone(l.Args)

381+

382+ got, err := l.ResolvedArgs(realShellResolver(nil))

383+ require.Error(t, err)

384+ require.Nil(t, got)

385+ require.Contains(t, err.Error(), "arg 1")

386+ require.Equal(t, orig, l.Args, "receiver Args must be preserved")

387+ })

388+

389+ t.Run("nil args returns nil, no error", func(t *testing.T) {

390+ t.Parallel()

391+ l := LSPConfig{}

392+ got, err := l.ResolvedArgs(realShellResolver(nil))

393+ require.NoError(t, err)

394+ require.Nil(t, got)

395+ })

396+}

397+

398+// TestLSPConfig_ResolvedEnv_RealShell pins the LSP env contract:

399+// success expands $VAR, failure wraps with the key name, and the

400+// receiver map is never mutated. The shape is map[string]string

401+// (not the MCP []string form) because powernap.ClientConfig.Environment

402+// takes a map directly.

403+func TestLSPConfig_ResolvedEnv_RealShell(t *testing.T) {

404+ t.Parallel()

405+

406+ t.Run("success expands $VAR", func(t *testing.T) {

407+ t.Parallel()

408+ l := LSPConfig{Env: map[string]string{"GOPATH": "$HOME/go"}}

409+ r := realShellResolver(map[string]string{"HOME": "/home/tester"})

410+ got, err := l.ResolvedEnv(r)

411+ require.NoError(t, err)

412+ require.Equal(t, map[string]string{"GOPATH": "/home/tester/go"}, got)

413+ })

414+

415+ t.Run("failure identifies offending key", func(t *testing.T) {

416+ t.Parallel()

417+ l := LSPConfig{Env: map[string]string{

418+ "GOPATH": "$(false)",

419+ "OTHER": "literal",

420+ }}

421+ orig := maps.Clone(l.Env)

422+

423+ got, err := l.ResolvedEnv(realShellResolver(nil))

424+ require.Error(t, err)

425+ require.Nil(t, got)

426+ require.Contains(t, err.Error(), `env "GOPATH"`)

427+ require.Equal(t, orig, l.Env, "receiver Env must be preserved")

428+ })

429+

430+ t.Run("idempotent and non-mutating", func(t *testing.T) {

431+ t.Parallel()

432+ l := LSPConfig{Env: map[string]string{

433+ "A": "$(echo one)",

434+ "B": "literal",

435+ }}

436+ orig := maps.Clone(l.Env)

437+ r := realShellResolver(nil)

438+

439+ first, err := l.ResolvedEnv(r)

440+ require.NoError(t, err)

441+ second, err := l.ResolvedEnv(r)

442+ require.NoError(t, err)

443+ require.Equal(t, first, second)

444+ require.Equal(t, orig, l.Env, "receiver Env must be preserved")

445+ })

446+}

447+

448+// TestLSPConfig_IdentityResolver pins the client-mode contract: both

449+// ResolvedArgs and ResolvedEnv round-trip the template verbatim under

450+// IdentityResolver and never error on unset variables. Local

451+// expansion would double-expand when the server does its own — this

452+// has to stay a pure pass-through.

453+func TestLSPConfig_IdentityResolver(t *testing.T) {

454+ t.Parallel()

455+

456+ l := LSPConfig{

457+ Args: []string{"--root", "$LSP_ROOT", "$(vault read -f lsp)"},

458+ Env: map[string]string{

459+ "GOPATH": "$HOME/go",

460+ "TOKEN": "$(cat /run/secrets/x)",

461+ },

462+ }

463+ r := IdentityResolver()

464+

465+ args, err := l.ResolvedArgs(r)

466+ require.NoError(t, err)

467+ require.Equal(t, l.Args, args)

468+

469+ envs, err := l.ResolvedEnv(r)

470+ require.NoError(t, err)

471+ require.Equal(t, l.Env, envs)

472+}

473+

270474// TestMCPConfig_IdentityResolver pins the client-mode contract: every

271475// Resolved* method round-trips the template verbatim and never errors

272476// on unset variables. Local expansion would double-expand when the

internal/config/resolve_test.go−64

@@ -220,62 +220,6 @@ func TestScrubErrorMessage(t *testing.T) {

220220 })

221221}

222222

223−func TestEnvironmentVariableResolver_ResolveValue(t *testing.T) {

224− tests := []struct {

225− name string

226− value string

227− envVars map[string]string

228− expected string

229− expectError bool

230− }{

231− {

232− name: "non-variable string returns as-is",

233− value: "plain-string",

234− expected: "plain-string",

235− },

236− {

237− name: "environment variable resolution",

238− value: "$HOME",

239− envVars: map[string]string{"HOME": "/home/user"},

240− expected: "/home/user",

241− },

242− {

243− name: "environment variable with complex value",

244− value: "$PATH",

245− envVars: map[string]string{"PATH": "/usr/bin:/bin:/usr/local/bin"},

246− expected: "/usr/bin:/bin:/usr/local/bin",

247− },

248− {

249− name: "missing environment variable returns error",

250− value: "$MISSING_VAR",

251− envVars: map[string]string{},

252− expectError: true,

253− },

254− {

255− name: "empty environment variable returns error",

256− value: "$EMPTY_VAR",

257− envVars: map[string]string{"EMPTY_VAR": ""},

258− expectError: true,

259− },

260− }

261−

262− for _, tt := range tests {

263− t.Run(tt.name, func(t *testing.T) {

264− testEnv := env.NewFromMap(tt.envVars)

265− resolver := NewEnvironmentVariableResolver(testEnv)

266−

267− result, err := resolver.ResolveValue(tt.value)

268−

269− if tt.expectError {

270− require.Error(t, err)

271− } else {

272− require.NoError(t, err)

273− require.Equal(t, tt.expected, result)

274− }

275− })

276− }

277−}

278−

279223func TestNewShellVariableResolver(t *testing.T) {

280224 testEnv := env.NewFromMap(map[string]string{"TEST": "value"})

281225 resolver := NewShellVariableResolver(testEnv)

@@ -283,11 +227,3 @@ func TestNewShellVariableResolver(t *testing.T) {

283227 require.NotNil(t, resolver)

284228 require.Implements(t, (*VariableResolver)(nil), resolver)

285229}

286−

287−func TestNewEnvironmentVariableResolver(t *testing.T) {

288− testEnv := env.NewFromMap(map[string]string{"TEST": "value"})

289− resolver := NewEnvironmentVariableResolver(testEnv)

290−

291− require.NotNil(t, resolver)

292− require.Implements(t, (*VariableResolver)(nil), resolver)

293−}

internal/lsp/client.go+12−3

@@ -5,7 +5,6 @@ import (

55 "encoding/json"

66 "fmt"

77 "log/slog"

8− "maps"

98 "os"

109 "path/filepath"

1110 "slices"

@@ -169,11 +168,21 @@ func (c *Client) createPowernapClient() error {

169168 return fmt.Errorf("invalid lsp command: %w", err)

170169 }

171170

171+ args, err := c.config.ResolvedArgs(c.resolver)

172+ if err != nil {

173+ return fmt.Errorf("invalid lsp args: %w", err)

174+ }

175+

176+ envs, err := c.config.ResolvedEnv(c.resolver)

177+ if err != nil {

178+ return fmt.Errorf("invalid lsp env: %w", err)

179+ }

180+

172181 clientConfig := powernap.ClientConfig{

173182 Command: home.Long(command),

174− Args: c.config.Args,

183+ Args: args,

175184 RootURI: rootURI,

176− Environment: maps.Clone(c.config.Env),

185+ Environment: envs,

177186 Settings: c.config.Options,

178187 InitOptions: c.config.InitOptions,

179188 WorkspaceFolders: []protocol.WorkspaceFolder{

internal/lsp/client_test.go+37−1

@@ -26,7 +26,7 @@ func TestClient(t *testing.T) {

2626

2727 // Test creating a powernap client - this will likely fail with echo

2828 // but we can still test the basic structure

29− client, err := New(ctx, "test", cfg, config.NewEnvironmentVariableResolver(env.NewFromMap(map[string]string{

29+ client, err := New(ctx, "test", cfg, config.NewShellVariableResolver(env.NewFromMap(map[string]string{

3030 "THE_CMD": "echo",

3131 })), ".", false)

3232 if err != nil {

@@ -61,6 +61,42 @@ func TestClient(t *testing.T) {

6161 }

6262}

6363

64+// TestNew_ExpansionFailure_Args pins that a failing $(cmd) in LSP

65+// args surfaces as a load error prefixed "invalid lsp args:" and that

66+// no client is returned. Mirrors the MCP contract where expansion

67+// failure hard-stops transport creation rather than silently running

68+// with an empty or literal value.

69+func TestNew_ExpansionFailure_Args(t *testing.T) {

70+ t.Parallel()

71+

72+ cfg := config.LSPConfig{

73+ Command: "echo",

74+ Args: []string{"--root", "$(false)"},

75+ }

76+ resolver := config.NewShellVariableResolver(env.NewFromMap(map[string]string{}))

77+

78+ client, err := New(t.Context(), "test-args-fail", cfg, resolver, ".", false)

79+ require.Error(t, err)

80+ require.Nil(t, client, "client must not start when args expansion fails")

81+ require.Contains(t, err.Error(), "invalid lsp args")

82+}

83+

84+// TestNew_ExpansionFailure_Env pins the same contract for env values.

85+func TestNew_ExpansionFailure_Env(t *testing.T) {

86+ t.Parallel()

87+

88+ cfg := config.LSPConfig{

89+ Command: "echo",

90+ Env: map[string]string{"BAD": "$(false)"},

91+ }

92+ resolver := config.NewShellVariableResolver(env.NewFromMap(map[string]string{}))

93+

94+ client, err := New(t.Context(), "test-env-fail", cfg, resolver, ".", false)

95+ require.Error(t, err)

96+ require.Nil(t, client, "client must not start when env expansion fails")

97+ require.Contains(t, err.Error(), "invalid lsp env")

98+}

99+

64100func TestNilClient(t *testing.T) {

65101 t.Parallel()

66102

internal/shell/expand.go+33−8

@@ -7,6 +7,7 @@ import (

77 "io"

88 "os"

99 "strings"

10+ "sync/atomic"

1011

1112 "mvdan.cc/sh/v3/expand"

1213 "mvdan.cc/sh/v3/interp"

@@ -18,11 +19,28 @@ import (

1819// to be embedded in a failing inner command.

1920const maxInnerStderrBytes = 512

2021

22+// NoUnset controls whether ExpandValue treats unset variables as an

23+// error. Default false matches bash: $UNSET expands to "". Store true

24+// to re-enable strict mode globally. Not exposed in crush.json; this is

25+// an internal escape hatch in case the lenient default turns out to be

26+// the wrong call.

27+//

28+// Declared atomic because ExpandValue is invoked concurrently (multiple

29+// MCP / LSP / provider loads in flight at startup, hook execution, etc.)

30+// and an unsynchronised read/write pair is a data race under the Go

31+// memory model regardless of test-level happens-before reasoning. The

32+// atomic load on the hot path is negligible against the cost of parsing

33+// and running through mvdan.

34+//

35+// See PLAN.md Phase 2 design decisions #11 and #12 for the full

36+// rationale.

37+var NoUnset atomic.Bool

38+

2139// ExpandValue expands shell-style substitutions in a single config value.

2240//

2341// Supported constructs match the bash tool:

2442//

25−// - $VAR and ${VAR} (unset is an error; see nounset below).

43+// - $VAR and ${VAR}.

2644// - ${VAR:-default} / ${VAR:+alt} / ${VAR:?msg}.

2745// - $(command) with full quoting and nesting.

2846// - escaped and quoted strings ("...", '...').

@@ -32,9 +50,11 @@ const maxInnerStderrBytes = 512

3250// - Returns exactly one string. No field splitting, no globbing, no

3351// pathname generation. Multi-word command output is preserved

3452// verbatim; it is never split into multiple values.

35−// - Nounset is on: unset variables produce an error instead of

36−// expanding to the empty string. Use ${VAR:-default} to opt in to

37−// an empty fallback.

53+// - Nounset is off by default, matching bash: unset variables expand

54+// to "". Opt in to strict behaviour per-reference with

55+// ${VAR:?msg}, which errors loudly when VAR is unset regardless of

56+// the global toggle. Flip the global default via

57+// shell.NoUnset.Store(true) as an internal escape hatch.

3858// - Embedded whitespace and newlines in the input are preserved

3959// verbatim. Command substitution strips trailing newlines only

4060// (POSIX), never leading or internal whitespace.

@@ -63,22 +83,27 @@ func ExpandValue(ctx context.Context, value string, env []string) (string, error

6383 logger: noopLogger{},

6484 }

6585

86+ strict := NoUnset.Load()

87+

6688 var stderrBuf bytes.Buffer

6789 cfg := &expand.Config{

6890 Env: expand.ListEnviron(env...),

69− NoUnset: true,

91+ NoUnset: strict,

7092 CmdSubst: func(w io.Writer, cs *syntax.CmdSubst) error {

7193 stderrBuf.Reset()

72− runner, rerr := interp.New(

94+ runnerOpts := []interp.RunnerOption{

7395 interp.StdIO(nil, w, &stderrBuf),

7496 interp.Interactive(false),

7597 interp.Env(expand.ListEnviron(env...)),

7698 interp.Dir(s.cwd),

7799 interp.ExecHandlers(s.execHandlers()...),

100+ }

101+ if strict {

78102 // Match the outer NoUnset: an unset $VAR inside

79103 // $(...) is also an error, not a silent empty.

80− interp.Params("-u"),

81− )

104+ runnerOpts = append(runnerOpts, interp.Params("-u"))

105+ }

106+ runner, rerr := interp.New(runnerOpts...)

82107 if rerr != nil {

83108 return rerr

84109 }

internal/shell/expand_test.go+51−9

@@ -105,22 +105,25 @@ func TestExpandValue_Success(t *testing.T) {

105105func TestExpandValue_Errors(t *testing.T) {

106106 t.Parallel()

107107

108− t.Run("unset var is an error, not empty", func(t *testing.T) {

108+ t.Run("unset var expands to empty under lenient default", func(t *testing.T) {

109109 t.Parallel()

110− _, err := ExpandValue(t.Context(), "$MISSING", nil)

111− require.Error(t, err)

110+ got, err := ExpandValue(t.Context(), "$MISSING", nil)

111+ require.NoError(t, err)

112+ require.Equal(t, "", got)

112113 })

113114

114− t.Run("unset var inside braces is an error", func(t *testing.T) {

115+ t.Run("unset var inside braces expands to empty", func(t *testing.T) {

115116 t.Parallel()

116− _, err := ExpandValue(t.Context(), "${MISSING}", nil)

117− require.Error(t, err)

117+ got, err := ExpandValue(t.Context(), "${MISSING}", nil)

118+ require.NoError(t, err)

119+ require.Equal(t, "", got)

118120 })

119121

120− t.Run("unset var inside cmdsubst is an error", func(t *testing.T) {

122+ t.Run("unset var inside cmdsubst expands to empty", func(t *testing.T) {

121123 t.Parallel()

122− _, err := ExpandValue(t.Context(), `$(printf '%s' "$MISSING")`, nil)

123− require.Error(t, err)

124+ got, err := ExpandValue(t.Context(), `$(printf '%s' "$MISSING")`, nil)

125+ require.NoError(t, err)

126+ require.Equal(t, "", got)

124127 })

125128

126129 t.Run("bad syntax returns error", func(t *testing.T) {

@@ -173,6 +176,45 @@ func TestExpandValue_Errors(t *testing.T) {

173176 })

174177}

175178

179+// TestExpandValue_StrictToggle pins the NoUnset escape hatch: when a

180+// caller flips strict mode on, bare $UNSET must error instead of

181+// expanding to the empty string. Must not run in parallel: it mutates

182+// the package-level NoUnset atomic, so a parallel peer observing the

183+// flipped value would break the lenient default other tests assume.

184+func TestExpandValue_StrictToggle(t *testing.T) {

185+ NoUnset.Store(true)

186+ t.Cleanup(func() { NoUnset.Store(false) })

187+

188+ _, err := ExpandValue(t.Context(), "$UNSET", nil)

189+ require.Error(t, err)

190+

191+ _, err = ExpandValue(t.Context(), "${UNSET}", nil)

192+ require.Error(t, err)

193+

194+ _, err = ExpandValue(t.Context(), `$(printf '%s' "$UNSET")`, nil)

195+ require.Error(t, err)

196+}

197+

198+// TestExpandValue_RequiredOptIn pins the per-reference opt-in strict

199+// idiom ${VAR:?msg}: it must error whether or not the global NoUnset

200+// toggle is on, so config authors can mark individual credentials as

201+// required without flipping the global default.

202+func TestExpandValue_RequiredOptIn(t *testing.T) {

203+ t.Parallel()

204+

205+ _, err := ExpandValue(t.Context(), "${REQUIRED:?must be set}", nil)

206+ require.Error(t, err)

207+ require.Contains(t, err.Error(), "must be set")

208+

209+ got, err := ExpandValue(

210+ t.Context(),

211+ "${REQUIRED:?must be set}",

212+ []string{"REQUIRED=ok"},

213+ )

214+ require.NoError(t, err)

215+ require.Equal(t, "ok", got)

216+}

217+

176218func TestSanitizeStderr(t *testing.T) {

177219 t.Parallel()

178220

internal/skills/builtin/crush-config/SKILL.md+57−2

@@ -29,6 +29,58 @@ Crush uses JSON configuration files with the following priority (highest to lowe

2929

3030The `$schema` property enables IDE autocomplete but is optional.

3131

32+## Shell Expansion

33+

34+Crush runs selected string fields through an embedded bash-compatible

35+shell at load time, so values can pull from env vars, files, or helper

36+commands.

37+

38+Supported constructs (match the `bash` tool):

39+

40+- `$VAR` and `${VAR}`

41+- `${VAR:-default}`, `${VAR:+alt}`, `${VAR:?message}`

42+- `$(command)` with full quoting and nesting

43+- Single- and double-quoted strings, escapes

44+

45+Default semantics match bash: an unset variable expands to an empty

46+string, no error. A failing `$(command)` is always a hard error. For

47+required credentials, use `${VAR:?message}` so a missing variable

48+fails loudly at load time with your message.

49+

50+```json

51+{ "api_key": "${CODEBERG_TOKEN:?set CODEBERG_TOKEN}" }

52+```

53+

54+### Which fields expand

55+

56+| Surface | Expansion |

57+| --------------------------------------------------- | --------- |

58+| Provider `api_key`, `base_url`, `api_endpoint` | yes |

59+| Provider `extra_headers` | yes |

60+| Provider `extra_body` | **no** |

61+| MCP `command`, `args`, `env`, `headers`, `url` | yes |

62+| LSP `command`, `args`, `env` | yes |

63+| Hook `command` | runs via `sh -c`, not the resolver |

64+

65+`extra_body` is a JSON passthrough. If you need env-driven values in

66+a request body, put them in `extra_headers`, `api_key`, or

67+`base_url` instead.

68+

69+### Empty-resolved headers are dropped

70+

71+When a header value resolves to the empty string (unset variable,

72+`$(echo)`, or literal `""`), the header is omitted from the

73+outgoing request. This keeps optional env-gated headers like

74+`"OpenAI-Organization": "$OPENAI_ORG_ID"` working cleanly when the

75+var isn't set. Applies to MCP `headers` and provider `extra_headers`.

76+

77+### Security note

78+

79+`crush.json` is trusted code. Any `$(...)` in it runs at load time

80+with the invoking user's shell privileges, before the UI appears.

81+Don't launch Crush in a directory whose `crush.json` you haven't

82+reviewed.

83+

3284## Common Tasks

3385

3486- 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.

79131```

80132

81133- `type` (required): `openai`, `openai-compat`, or `anthropic`

82−- `api_key` supports `$ENV_VAR` syntax.

134+- `api_key`, `base_url`, `api_endpoint`, and `extra_headers` are shell-expanded (see [Shell Expansion](#shell-expansion)).

135+- `extra_body` is a JSON passthrough and is **not** expanded.

83136- Additional fields: `disable`, `system_prompt_prefix`, `extra_headers`, `extra_body`, `provider_options`.

84137

85138## LSP Configuration

@@ -89,7 +142,7 @@ The `$schema` property enables IDE autocomplete but is optional.

89142 "lsp": {

90143 "go": {

91144 "command": "gopls",

92− "env": { "GOTOOLCHAIN": "go1.24.5" }

145+ "env": { "GOPATH": "$HOME/go" }

93146 },

94147 "typescript": {

95148 "command": "typescript-language-server",

@@ -100,6 +153,7 @@ The `$schema` property enables IDE autocomplete but is optional.

100153```

101154

102155- `command` (required), `args`, `env` cover most setups.

156+- `command`, `args`, and `env` values are shell-expanded (see [Shell Expansion](#shell-expansion)).

103157- Additional fields: `disabled`, `filetypes`, `root_markers`, `init_options`, `options`, `timeout`.

104158

105159## MCP Servers

@@ -124,6 +178,7 @@ The `$schema` property enables IDE autocomplete but is optional.

124178```

125179

126180- `type` (required): `stdio`, `sse`, or `http`

181+- `command`, `args`, `env`, `headers`, and `url` are shell-expanded (see [Shell Expansion](#shell-expansion)).

127182- Additional fields: `env`, `disabled`, `disabled_tools`, `timeout`.

128183

129184## Options