f23e998 · crush · git.iko.soy

git.iko.soy/crush

on main help sign in

config: fail provider header expansion loudly and drop empty values

f23e998Christian Rocha6 months ago

internal/config/config.go+10

compare with main

unifiedsplit

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

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

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

370//

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

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

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

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

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

376+// Phase 2 design decision #18.

377+//

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

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

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

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

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

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

@@ -386,6 +393,9 @@ func (m MCPConfig) ResolvedHeaders(r VariableResolver) (map[string]string, error

386 if err != nil {

393 if err != nil {

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

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

388 }

395 }

396+ if v == "" {

397+ continue

398+ }

389 out[k] = v

399 out[k] = v

390 }

400 }

391 return out, nil

401 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+118−34

@@ -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,17 +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 — under Phase 2 lenient nounset this

1621− // resolves cleanly to "" rather than erroring;

1622− // Step 10 will rewrite this whole test once the

1623− // empty-drop rule replaces the log-and-continue

1624− // loop at load.go:225.

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

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

1627− // failure in X-Broken did not abort the loop.

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

1629 },

1616 },

1630 },

1617 },

1631 }),

1618 }),

@@ -1639,20 +1626,117 @@ func TestConfig_configureProviders_ProviderHeaderResolveFailure(t *testing.T) {

1639 resolver := NewShellVariableResolver(testEnv)

1626 resolver := NewShellVariableResolver(testEnv)

1640

1627

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

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

1642− 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")

1643

1663

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

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

1645 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", "")

1646

1691

1647− // X-Broken still errors on $(false) so its literal template is

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

1648− // preserved by the log-and-continue loop. X-Missing resolves

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

1649− // cleanly to "" under lenient nounset: the current divergence

1694+ })

1650− // only kicks in on resolver errors, so a non-erroring empty

1695+ resolver := NewShellVariableResolver(testEnv)

1651− // expansion writes through. The happy-path header is resolved

1696+

1652− // through the shell (pass-through for a literal value). Step 10

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

1653− // will flip this whole surface to the MCP contract plus the

1698+ require.NoError(t, err)

1654− // empty-drop rule from design decision #18.

1699+

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

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)

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

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

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

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

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

1658}

1742}

internal/config/resolve_real_test.go+65

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

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

260}

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+

262// TestResolvedArgs_RealShell exercises both success and failure for

327// TestResolvedArgs_RealShell exercises both success and failure for

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

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

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

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