Skip to content

fix: surface api_key/base_url resolution errors instead of discarding them#3292

Open
albatrossflyon-coder wants to merge 1 commit into
charmbracelet:mainfrom
albatrossflyon-coder:fix-resolve-error-discarded-3011
Open

fix: surface api_key/base_url resolution errors instead of discarding them#3292
albatrossflyon-coder wants to merge 1 commit into
charmbracelet:mainfrom
albatrossflyon-coder:fix-resolve-error-discarded-3011

Conversation

@albatrossflyon-coder

Copy link
Copy Markdown

Summary

  • buildProvider discarded the error from c.cfg.Resolve() for both api_key and base_url with the blank identifier (apiKey, _ := ...)
  • A malformed template, a failed \$(command) substitution, or a required \${VAR:?message} check firing all failed silently — the provider was then built with an empty/wrong value, and the resulting auth failure surfaced far from the real root cause, deep inside the provider's own request path instead of at config load time
  • Fix: check and return both errors, wrapped with which field and provider ID failed

Deliberately scoped narrower than the issue's literal suggestion: the issue proposes failing whenever api_key resolves to empty, but that would be a real regression — Bedrock (AWS credential/profile auth) and Azure OpenAI with Entra ID both legitimately have no api_key configured. crush's resolver already supports opting into a hard failure per-provider via \${VAR:?message} syntax in the provider's own api_key template (documented in internal/config/resolve.go), so the actual bug here is the discarded resolution error, not missing empty-string validation.

Fixes #3011

Test plan

  • go build ./...
  • go test ./internal/agent/... — all pass, no regressions (pre-existing unrelated Windows path-separator failure in internal/agent/tools confirmed present on unmodified main too)
  • Added TestBuildProviderSurfacesResolveErrors — confirms both api_key and base_url resolution failures (via \${VAR:?message}) now return an error naming the field and provider, instead of silently proceeding
  • Semgrep (p/security-audit, p/secrets, p/golang, Trail of Bits Go rules) on the changed files — 0 findings

… them

buildProvider discarded the error from c.cfg.Resolve() for both
api_key and base_url with the blank identifier, so a malformed
template, a failed $(command) substitution, or a required
${VAR:?message} check firing all failed silently. The provider would
then be built with an empty/wrong value, and any resulting auth
failure surfaced far from the actual root cause deep inside the
provider's own request path instead of at config load time.

Note: this intentionally does not add a blanket "empty api_key is
invalid" check — Bedrock (AWS credentials/profile) and Azure OpenAI
with Entra ID both legitimately have no api_key, and crush's config
resolver already supports opting into a hard failure per-provider via
${VAR:?message} syntax in the provider's own api_key template. The
bug is specifically the discarded resolution error, not missing
key-emptiness validation.

Fixes charmbracelet#3011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing api_key causes provder to fail with malformed Authorization header

1 participant