8f2fd3d · crush · git.iko.soy

git.iko.soy/crush

on main help sign in

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

8f2fd3dChristian Rocha5 months ago

README.md+25−6

compare with main

unifiedsplit

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

298for Server-Sent Events.

299

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

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

301+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

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

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

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

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

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

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

305+

306−a default.

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.

307

326

308```json

327```json

309{

328{

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

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

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

92 })

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) {

95 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.

96 m := config.MCPConfig{

100 m := config.MCPConfig{

97 Type: config.MCPHttp,

101 Type: config.MCPHttp,

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

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

99 }

103 }

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

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

101 require.Error(t, err)

105 require.Error(t, err)

102 require.Nil(t, tr)

106 require.Nil(t, tr)

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

107 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)

105 })

128 })

106

129

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

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

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

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

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

202 })

225 })

203

226

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) {

205 t.Parallel()

228 t.Parallel()

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

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

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

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

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

209 r := shellResolverWithPath(t, nil)

234 r := shellResolverWithPath(t, nil)

210 m := config.MCPConfig{

235 m := config.MCPConfig{

211 Type: config.MCPStdio,

236 Type: config.MCPStdio,

212 Command: "forgejo-mcp",

237 Command: "forgejo-mcp",

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

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

214 }

239 }

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

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

216 require.Error(t, err)

241 require.Error(t, err)

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

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

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

219 })

244 })

220

245

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+

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

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

222 t.Parallel()

269 t.Parallel()

223 r := shellResolverWithPath(t, nil)

270 r := shellResolverWithPath(t, nil)

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

311

358

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

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

313 t.Parallel()

360 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.

314 r := shellResolverWithPath(t, nil)

365 r := shellResolverWithPath(t, nil)

315 m := config.MCPConfig{

366 m := config.MCPConfig{

316 Type: config.MCPSSE,

367 Type: config.MCPSSE,

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

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

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

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

319 }

370 }

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

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

321 require.Error(t, err)

372 require.Error(t, err)

322 require.Nil(t, tr)

373 require.Nil(t, tr)

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

374 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")

325 })

398 })

326}

399}

327

400

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

356 wantErrContains: "env FORGEJO_ACCESS_TOKEN",

429 wantErrContains: "env FORGEJO_ACCESS_TOKEN",

357 },

430 },

358 {

431 {

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).

359 name: "stdio args failure",

435 name: "stdio args failure",

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

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

361 cfg: config.MCPConfig{

437 cfg: config.MCPConfig{

362 Type: config.MCPStdio,

438 Type: config.MCPStdio,

363 Command: "echo",

439 Command: "echo",

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

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

365 },

441 },

366 wantErrContains: "arg 1",

442 wantErrContains: "arg 1",

367 },

443 },

368 {

444 {

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

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

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

369 name: "http url failure",

448 name: "http url failure",

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

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

371 cfg: config.MCPConfig{

450 cfg: config.MCPConfig{

372 Type: config.MCPHttp,

451 Type: config.MCPHttp,

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

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

374 },

453 },

375 wantErrContains: "url:",

454 wantErrContains: "url:",

376 },

455 },

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+ },

377 {

470 {

378 name: "http header failure",

471 name: "http header failure",

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

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

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

394 wantErrContains: "url:",

487 wantErrContains: "url:",

395 },

488 },

396 {

489 {

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.

397 name: "sse header failure",

495 name: "sse header failure",

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

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

399 cfg: config.MCPConfig{

497 cfg: config.MCPConfig{

400 Type: config.MCPSSE,

498 Type: config.MCPSSE,

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

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

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

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

403 },

501 },

404 wantErrContains: "header Authorization",

502 wantErrContains: "header Authorization",

405 },

503 },

internal/config/config.go+102−3

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

108 // Custom system prompt prefix.

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

110

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.

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

118 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.

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

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

115

128

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

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

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

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

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

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

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

179

192

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.

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

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

182}

200}

183

201

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

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

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

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

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

373//

391//

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+//

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

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

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

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

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

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

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

389 if err != nil {

414 if err != nil {

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

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

391 }

416 }

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+ }

392 out[k] = v

491 out[k] = v

393 }

492 }

394 return out, nil

493 return out, nil

internal/config/load.go+17−8

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

222 if len(config.ExtraHeaders) > 0 {

223 maps.Copy(headers, config.ExtraHeaders)

224 }

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

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

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

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

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

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

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

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

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

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

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

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

231 for k, v := range headers {

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

233 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)

235 continue

238 continue

236 }

239 }

237 headers[k] = resolved

240 headers[k] = resolved

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

382 continue

385 continue

383 }

386 }

384

387

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.

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

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

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

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

387 if err != nil {

393 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)

389 continue

398 continue

390 }

399 }

391 providerConfig.ExtraHeaders[k] = resolved

400 providerConfig.ExtraHeaders[k] = resolved

internal/config/load_test.go+278−64

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

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

78 "OPENAI_API_KEY": "test-key",

79 })

80− resolver := NewEnvironmentVariableResolver(env)

80+ resolver := NewShellVariableResolver(env)

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

82 require.NoError(t, err)

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

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

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

121 "OPENAI_API_KEY": "test-key",

122 })

123− resolver := NewEnvironmentVariableResolver(env)

123+ resolver := NewShellVariableResolver(env)

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

125 require.NoError(t, err)

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

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

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

163 "OPENAI_API_KEY": "test-key",

164 })

165− resolver := NewEnvironmentVariableResolver(env)

165+ resolver := NewShellVariableResolver(env)

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

167 require.NoError(t, err)

168 // Should be to because of the env variable

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

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

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

200 })

201− resolver := NewEnvironmentVariableResolver(env)

201+ resolver := NewShellVariableResolver(env)

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

203 require.NoError(t, err)

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

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

224 cfg := &Config{}

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

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

227− resolver := NewEnvironmentVariableResolver(env)

227+ resolver := NewShellVariableResolver(env)

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

229 require.NoError(t, err)

230 // Provider should not be configured without credentials

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

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

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

251 })

252− resolver := NewEnvironmentVariableResolver(env)

252+ resolver := NewShellVariableResolver(env)

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

254 require.Error(t, err)

255}

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

272 "VERTEXAI_PROJECT": "test-project",

273 "VERTEXAI_LOCATION": "us-central1",

274 })

275− resolver := NewEnvironmentVariableResolver(env)

275+ resolver := NewShellVariableResolver(env)

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

277 require.NoError(t, err)

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

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

304 "GOOGLE_CLOUD_PROJECT": "test-project",

305 "GOOGLE_CLOUD_LOCATION": "us-central1",

306 })

307− resolver := NewEnvironmentVariableResolver(env)

307+ resolver := NewShellVariableResolver(env)

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

309 require.NoError(t, err)

310 // Provider should not be configured without proper credentials

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

329 "GOOGLE_GENAI_USE_VERTEXAI": "true",

330 "GOOGLE_CLOUD_LOCATION": "us-central1",

331 })

332− resolver := NewEnvironmentVariableResolver(env)

332+ resolver := NewShellVariableResolver(env)

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

334 require.NoError(t, err)

335 // Provider should not be configured without project

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

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

354 "OPENAI_API_KEY": "test-key",

355 })

356− resolver := NewEnvironmentVariableResolver(env)

356+ resolver := NewShellVariableResolver(env)

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

358 require.NoError(t, err)

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

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

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

545 "OPENAI_API_KEY": "test-key",

546 })

547− resolver := NewEnvironmentVariableResolver(env)

547+ resolver := NewShellVariableResolver(env)

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

549 require.NoError(t, err)

550

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

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

573

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

575− resolver := NewEnvironmentVariableResolver(env)

575+ resolver := NewShellVariableResolver(env)

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

577 require.NoError(t, err)

578

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

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

596

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

598− resolver := NewEnvironmentVariableResolver(env)

598+ resolver := NewShellVariableResolver(env)

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

600 require.NoError(t, err)

601

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

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

618

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

620− resolver := NewEnvironmentVariableResolver(env)

620+ resolver := NewShellVariableResolver(env)

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

622 require.NoError(t, err)

623

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

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

643

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

645− resolver := NewEnvironmentVariableResolver(env)

645+ resolver := NewShellVariableResolver(env)

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

647 require.NoError(t, err)

648

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

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

668

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

670− resolver := NewEnvironmentVariableResolver(env)

670+ resolver := NewShellVariableResolver(env)

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

672 require.NoError(t, err)

673

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

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

696

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

698− resolver := NewEnvironmentVariableResolver(env)

698+ resolver := NewShellVariableResolver(env)

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

700 require.NoError(t, err)

701

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

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

726

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

728− resolver := NewEnvironmentVariableResolver(env)

728+ resolver := NewShellVariableResolver(env)

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

730 require.NoError(t, err)

731

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

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

761 "GOOGLE_GENAI_USE_VERTEXAI": "false",

762 })

763− resolver := NewEnvironmentVariableResolver(env)

763+ resolver := NewShellVariableResolver(env)

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

765 require.NoError(t, err)

766

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

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

792

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

794− resolver := NewEnvironmentVariableResolver(env)

794+ resolver := NewShellVariableResolver(env)

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

796 require.NoError(t, err)

797

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

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

823

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

825− resolver := NewEnvironmentVariableResolver(env)

825+ resolver := NewShellVariableResolver(env)

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

827 require.NoError(t, err)

828

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

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

856 "OPENAI_API_KEY": "test-key",

857 })

858− resolver := NewEnvironmentVariableResolver(env)

858+ resolver := NewShellVariableResolver(env)

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

860 require.NoError(t, err)

861

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

889 cfg := &Config{}

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

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

892− resolver := NewEnvironmentVariableResolver(env)

892+ resolver := NewShellVariableResolver(env)

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

894 require.NoError(t, err)

895

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

925 cfg := &Config{}

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

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

928− resolver := NewEnvironmentVariableResolver(env)

928+ resolver := NewShellVariableResolver(env)

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

930 require.NoError(t, err)

931

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

955 cfg := &Config{}

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

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

958− resolver := NewEnvironmentVariableResolver(env)

958+ resolver := NewShellVariableResolver(env)

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

960 require.NoError(t, err)

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

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

998 }

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

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

1001− resolver := NewEnvironmentVariableResolver(env)

1001+ resolver := NewShellVariableResolver(env)

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

1003 require.NoError(t, err)

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

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

1042 }

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

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

1045− resolver := NewEnvironmentVariableResolver(env)

1045+ resolver := NewShellVariableResolver(env)

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

1047 require.NoError(t, err)

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

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

1084 }

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

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

1087− resolver := NewEnvironmentVariableResolver(env)

1087+ resolver := NewShellVariableResolver(env)

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

1089 require.NoError(t, err)

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

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

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

1130 "OPENAI_API_KEY": "test-key",

1131 })

1132− resolver := NewEnvironmentVariableResolver(env)

1132+ resolver := NewShellVariableResolver(env)

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

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

1135

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

1172 "MY_API_KEY": "test-key",

1173 "OPENAI_API_KEY": "test-key",

1174 })

1175− resolver := NewEnvironmentVariableResolver(env)

1175+ resolver := NewShellVariableResolver(env)

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

1177 require.NoError(t, err)

1178

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

1226 "OPENAI_API_KEY": "test-key",

1227 "ANTHROPIC_API_KEY": "test-key",

1228 })

1229− resolver := NewEnvironmentVariableResolver(env)

1229+ resolver := NewShellVariableResolver(env)

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

1231 require.NoError(t, err)

1232

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

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

1255

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

1257− resolver := NewEnvironmentVariableResolver(env)

1257+ resolver := NewShellVariableResolver(env)

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

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

1260

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

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

1279

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

1281− resolver := NewEnvironmentVariableResolver(env)

1281+ resolver := NewShellVariableResolver(env)

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

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

1284

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

1336 cfg.setDefaults(dir, "")

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

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

1339− resolver := NewEnvironmentVariableResolver(env)

1339+ resolver := NewShellVariableResolver(env)

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

1341 require.NoError(t, err)

1342

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

1386 }

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

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

1389− resolver := NewEnvironmentVariableResolver(env)

1389+ resolver := NewShellVariableResolver(env)

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

1391 require.NoError(t, err)

1392

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

1448 }

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

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

1451− resolver := NewEnvironmentVariableResolver(env)

1451+ resolver := NewShellVariableResolver(env)

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

1453 require.NoError(t, err)

1454

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

1493 }

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

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

1496− resolver := NewEnvironmentVariableResolver(env)

1496+ resolver := NewShellVariableResolver(env)

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

1498 require.NoError(t, err)

1499

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

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

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

1534 })

1535− resolver := NewEnvironmentVariableResolver(env)

1535+ resolver := NewShellVariableResolver(env)

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

1537 require.NoError(t, err)

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

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

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

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

1581 })

1582− resolver := NewEnvironmentVariableResolver(env)

1582+ resolver := NewShellVariableResolver(env)

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

1584 require.NoError(t, err)

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

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

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

1591}

1592

1593−// TestConfig_configureProviders_ProviderHeaderResolveFailure pins the

1593+// TestConfig_configureProviders_ProviderHeaderResolveError pins

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

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

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

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

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

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

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

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

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

1598+// contract.

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

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

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) {

1605 knownProviders := []catwalk.Provider{

1600 knownProviders := []catwalk.Provider{

1606 {

1601 {

1607 ID: "openai",

1602 ID: "openai",

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

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

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

1616 "openai": {

1611 "openai": {

1617 ExtraHeaders: map[string]string{

1612 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.

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

1615 "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",

1625 },

1616 },

1626 },

1617 },

1627 }),

1618 }),

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

1635 resolver := NewShellVariableResolver(testEnv)

1626 resolver := NewShellVariableResolver(testEnv)

1636

1627

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

1628 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")

1639

1663

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

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

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

1665 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)

1642

1859

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

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

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

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

1645− // for a literal value).

1862+ require.False(t, exists)

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"])

1649}

1863}

internal/config/mcp_resolved_url_test.go+20−3

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

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

45 })

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) {

48 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}.

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

55 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"}

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

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

51 require.Error(t, err)

69 require.Error(t, err)

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

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

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

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

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

55 })

72 })

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

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

internal/config/resolve.go+12−31

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

10)

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

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

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

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

15const resolveTimeout = 5 * time.Minute

14const resolveTimeout = 5 * time.Minute

17type VariableResolver interface {

16type VariableResolver interface {

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

58// NewShellVariableResolver returns a VariableResolver that delegates to

57// NewShellVariableResolver returns a VariableResolver that delegates to

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

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

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

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

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

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

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

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.

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

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

64 r := &shellVariableResolver{

66 r := &shellVariableResolver{

65 env: e,

67 env: e,

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

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

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

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

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

79//

81//

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

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

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

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

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

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.

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

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

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

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

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

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

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

167 }

172 }

168 return string(out)

173 return string(out)

169}

174}

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) {

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

160}

161

162−// TestResolvedEnv_RealShell_NounsetRegression is the single most

162+// TestResolvedEnv_RealShell_UnsetExpandsEmpty pins Phase 2's lenient

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

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

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

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

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

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

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

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

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

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) {

168 t.Parallel()

171 t.Parallel()

169

172

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

173 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.

172 "FORGEJO_ACCESS_TOKEN": "$FORGJO_TOKEN",

177 "FORGEJO_ACCESS_TOKEN": "$FORGJO_TOKEN",

173 }}

178 }}

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

179 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")

176 require.Nil(t, got)

198 require.Nil(t, got)

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()

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

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

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

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

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

179}

206}

180

207

181// TestResolvedEnv_RealShell_FailureDetail pins that a failing inner

208// TestResolvedEnv_RealShell_FailureDetail pins that a failing inner

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

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

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

233}

260}

234

261

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+

235// TestResolvedArgs_RealShell exercises both success and failure for

327// TestResolvedArgs_RealShell exercises both success and failure for

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

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

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

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

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

267 })

359 })

268}

360}

269

361

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+

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

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

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

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

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

476// 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) {

220 })

221}

222

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−

279func TestNewShellVariableResolver(t *testing.T) {

223func TestNewShellVariableResolver(t *testing.T) {

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

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

281 resolver := NewShellVariableResolver(testEnv)

225 resolver := NewShellVariableResolver(testEnv)

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

283 require.NotNil(t, resolver)

227 require.NotNil(t, resolver)

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

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

285}

229}

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 (

5 "encoding/json"

6 "fmt"

7 "log/slog"

8− "maps"

9 "os"

8 "os"

10 "path/filepath"

9 "path/filepath"

11 "slices"

10 "slices"

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

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

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

170 }

169 }

171

170

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+

172 clientConfig := powernap.ClientConfig{

181 clientConfig := powernap.ClientConfig{

173 Command: home.Long(command),

182 Command: home.Long(command),

174− Args: c.config.Args,

183+ Args: args,

175 RootURI: rootURI,

184 RootURI: rootURI,

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

185+ Environment: envs,

177 Settings: c.config.Options,

186 Settings: c.config.Options,

178 InitOptions: c.config.InitOptions,

187 InitOptions: c.config.InitOptions,

179 WorkspaceFolders: []protocol.WorkspaceFolder{

188 WorkspaceFolders: []protocol.WorkspaceFolder{

internal/lsp/client_test.go+37−1

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

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

28 // 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{

30 "THE_CMD": "echo",

31 })), ".", false)

32 if err != nil {

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

61 }

62}

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+

64func TestNilClient(t *testing.T) {

100func TestNilClient(t *testing.T) {

65 t.Parallel()

101 t.Parallel()

102

internal/shell/expand.go+33−8

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

7 "io"

8 "os"

9 "strings"

10+ "sync/atomic"

11 "mvdan.cc/sh/v3/expand"

12 "mvdan.cc/sh/v3/expand"

12 "mvdan.cc/sh/v3/interp"

13 "mvdan.cc/sh/v3/interp"

@@ -18,11 +19,28 @@ import (

18// to be embedded in a failing inner command.

19// to be embedded in a failing inner command.

19const maxInnerStderrBytes = 512

20const maxInnerStderrBytes = 512

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+

21// ExpandValue expands shell-style substitutions in a single config value.

39// ExpandValue expands shell-style substitutions in a single config value.

22//

40//

23// Supported constructs match the bash tool:

41// Supported constructs match the bash tool:

24//

42//

25−// - $VAR and ${VAR} (unset is an error; see nounset below).

43+// - $VAR and ${VAR}.

26// - ${VAR:-default} / ${VAR:+alt} / ${VAR:?msg}.

44// - ${VAR:-default} / ${VAR:+alt} / ${VAR:?msg}.

27// - $(command) with full quoting and nesting.

45// - $(command) with full quoting and nesting.

28// - escaped and quoted strings ("...", '...').

46// - escaped and quoted strings ("...", '...').

@@ -32,9 +50,11 @@ const maxInnerStderrBytes = 512

32// - Returns exactly one string. No field splitting, no globbing, no

50// - Returns exactly one string. No field splitting, no globbing, no

33// pathname generation. Multi-word command output is preserved

51// pathname generation. Multi-word command output is preserved

34// verbatim; it is never split into multiple values.

52// verbatim; it is never split into multiple values.

35−// - Nounset is on: unset variables produce an error instead of

53+// - Nounset is off by default, matching bash: unset variables expand

36−// expanding to the empty string. Use ${VAR:-default} to opt in to

54+// to "". Opt in to strict behaviour per-reference with

37−// an empty fallback.

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.

38// - Embedded whitespace and newlines in the input are preserved

58// - Embedded whitespace and newlines in the input are preserved

39// verbatim. Command substitution strips trailing newlines only

59// verbatim. Command substitution strips trailing newlines only

40// (POSIX), never leading or internal whitespace.

60// (POSIX), never leading or internal whitespace.

@@ -63,22 +83,27 @@ func ExpandValue(ctx context.Context, value string, env []string) (string, error

63 logger: noopLogger{},

83 logger: noopLogger{},

64 }

84 }

86+ strict := NoUnset.Load()

87+

66 var stderrBuf bytes.Buffer

88 var stderrBuf bytes.Buffer

67 cfg := &expand.Config{

89 cfg := &expand.Config{

68 Env: expand.ListEnviron(env...),

90 Env: expand.ListEnviron(env...),

69− NoUnset: true,

91+ NoUnset: strict,

70 CmdSubst: func(w io.Writer, cs *syntax.CmdSubst) error {

92 CmdSubst: func(w io.Writer, cs *syntax.CmdSubst) error {

71 stderrBuf.Reset()

93 stderrBuf.Reset()

72− runner, rerr := interp.New(

94+ runnerOpts := []interp.RunnerOption{

73 interp.StdIO(nil, w, &stderrBuf),

95 interp.StdIO(nil, w, &stderrBuf),

74 interp.Interactive(false),

96 interp.Interactive(false),

75 interp.Env(expand.ListEnviron(env...)),

97 interp.Env(expand.ListEnviron(env...)),

76 interp.Dir(s.cwd),

98 interp.Dir(s.cwd),

77 interp.ExecHandlers(s.execHandlers()...),

99 interp.ExecHandlers(s.execHandlers()...),

100+ }

101+ if strict {

78 // Match the outer NoUnset: an unset $VAR inside

102 // Match the outer NoUnset: an unset $VAR inside

79 // $(...) is also an error, not a silent empty.

103 // $(...) is also an error, not a silent empty.

80− interp.Params("-u"),

104+ runnerOpts = append(runnerOpts, interp.Params("-u"))

81− )

105+ }

106+ runner, rerr := interp.New(runnerOpts...)

82 if rerr != nil {

107 if rerr != nil {

83 return rerr

108 return rerr

84 }

109 }

internal/shell/expand_test.go+51−9

@@ -105,22 +105,25 @@ func TestExpandValue_Success(t *testing.T) {

105func TestExpandValue_Errors(t *testing.T) {

106 t.Parallel()

107

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) {

109 t.Parallel()

110− _, err := ExpandValue(t.Context(), "$MISSING", nil)

110+ got, err := ExpandValue(t.Context(), "$MISSING", nil)

111− require.Error(t, err)

111+ require.NoError(t, err)

112+ require.Equal(t, "", got)

112 })

113 })

113

114

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) {

115 t.Parallel()

116 t.Parallel()

116− _, err := ExpandValue(t.Context(), "${MISSING}", nil)

117+ got, err := ExpandValue(t.Context(), "${MISSING}", nil)

117− require.Error(t, err)

118+ require.NoError(t, err)

119+ require.Equal(t, "", got)

118 })

120 })

119

121

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) {

121 t.Parallel()

123 t.Parallel()

122− _, err := ExpandValue(t.Context(), `$(printf '%s' "$MISSING")`, nil)

124+ got, err := ExpandValue(t.Context(), `$(printf '%s' "$MISSING")`, nil)

123− require.Error(t, err)

125+ require.NoError(t, err)

126+ require.Equal(t, "", got)

124 })

127 })

125

128

126 t.Run("bad syntax returns error", func(t *testing.T) {

129 t.Run("bad syntax returns error", func(t *testing.T) {

@@ -173,6 +176,45 @@ func TestExpandValue_Errors(t *testing.T) {

173 })

176 })

174}

177}

175

178

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+

176func TestSanitizeStderr(t *testing.T) {

218func TestSanitizeStderr(t *testing.T) {

177 t.Parallel()

219 t.Parallel()

178

220

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

30The `$schema` property enables IDE autocomplete but is optional.

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+

32## Common Tasks

84## Common Tasks

34- Add a custom provider: add an entry under `providers` with `type`, `base_url`, `api_key`, and `models`.

86- 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.

79```

131```

132

81- `type` (required): `openai`, `openai-compat`, or `anthropic`

133- `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.

83- Additional fields: `disable`, `system_prompt_prefix`, `extra_headers`, `extra_body`, `provider_options`.

136- Additional fields: `disable`, `system_prompt_prefix`, `extra_headers`, `extra_body`, `provider_options`.

137

85## LSP Configuration

138## LSP Configuration

@@ -89,7 +142,7 @@ The `$schema` property enables IDE autocomplete but is optional.

89 "lsp": {

142 "lsp": {

90 "go": {

143 "go": {

91 "command": "gopls",

144 "command": "gopls",

92− "env": { "GOTOOLCHAIN": "go1.24.5" }

145+ "env": { "GOPATH": "$HOME/go" }

93 },

146 },

94 "typescript": {

147 "typescript": {

95 "command": "typescript-language-server",

148 "command": "typescript-language-server",

@@ -100,6 +153,7 @@ The `$schema` property enables IDE autocomplete but is optional.

100```

153```

101

154

102- `command` (required), `args`, `env` cover most setups.

155- `command` (required), `args`, `env` cover most setups.

156+- `command`, `args`, and `env` values are shell-expanded (see [Shell Expansion](#shell-expansion)).

103- Additional fields: `disabled`, `filetypes`, `root_markers`, `init_options`, `options`, `timeout`.

157- Additional fields: `disabled`, `filetypes`, `root_markers`, `init_options`, `options`, `timeout`.

104

158

105## MCP Servers

159## MCP Servers

@@ -124,6 +178,7 @@ The `$schema` property enables IDE autocomplete but is optional.

124```

178```

125

179

126- `type` (required): `stdio`, `sse`, or `http`

180- `type` (required): `stdio`, `sse`, or `http`

181+- `command`, `args`, `env`, `headers`, and `url` are shell-expanded (see [Shell Expansion](#shell-expansion)).

127- Additional fields: `env`, `disabled`, `disabled_tools`, `timeout`.

182- Additional fields: `env`, `disabled`, `disabled_tools`, `timeout`.

128

183

129## Options

184## Options