docs: align Hugging Face provider docs, errors, and tests with shipped route#2879
docs: align Hugging Face provider docs, errors, and tests with shipped route#2879mvanhorn wants to merge 2 commits into
Conversation
|
Thanks @mvanhorn for taking the time to contribute. This repository is currently observing a maintainer-managed contribution gate in dry-run mode, so this pull request is staying open. When enforcement is enabled, pull requests from contributors who are not listed in Please read |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for Hugging Face Inference Providers as an OpenAI-compatible router provider, introducing aliases (huggingface, hugging-face, hugging_face, hf) and environment variable fallbacks for API keys, base URLs, and models. The changes also update documentation, add unit tests, and extend the provider registry validation script. The reviewer identified two high-severity issues in crates/tui/src/config.rs where empty environment variables (such as HUGGINGFACE_BASE_URL or HUGGINGFACE_MODEL set to empty strings) would return Ok("") and bypass the intended fallback logic to HF_BASE_URL or HF_MODEL. Using .ok().filter(...) was suggested to ensure empty strings are treated as None and correctly trigger the fallbacks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| && let Ok(value) = | ||
| std::env::var("HUGGINGFACE_BASE_URL").or_else(|_| std::env::var("HF_BASE_URL")) | ||
| && !value.trim().is_empty() |
There was a problem hiding this comment.
If HUGGINGFACE_BASE_URL is set to an empty string, std::env::var returns Ok(""). Because it is Ok, the or_else fallback is not evaluated, and the empty string is rejected by the subsequent !value.trim().is_empty() check, completely skipping the fallback to HF_BASE_URL.\n\nUsing .ok().filter(...) ensures that an empty primary variable is treated as None, correctly triggering the fallback.
&& let Some(value) = std::env::var("HUGGINGFACE_BASE_URL")\n .ok()\n .filter(|v| !v.trim().is_empty())\n .or_else(|| std::env::var("HF_BASE_URL").ok().filter(|v| !v.trim().is_empty()))| && let Ok(value) = std::env::var("HUGGINGFACE_MODEL").or_else(|_| std::env::var("HF_MODEL")) | ||
| && !value.trim().is_empty() |
There was a problem hiding this comment.
Similar to the base URL fallback, if HUGGINGFACE_MODEL is set to an empty string, the or_else fallback to HF_MODEL will not be evaluated.\n\nUsing .ok().filter(...) ensures that an empty primary variable is treated as None, correctly triggering the fallback.
&& let Some(value) = std::env::var("HUGGINGFACE_MODEL")\n .ok()\n .filter(|v| !v.trim().is_empty())\n .or_else(|| std::env::var("HF_MODEL").ok().filter(|v| !v.trim().is_empty()))![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| #[must_use] | ||
| pub fn all() -> &'static [Self] { | ||
| &[ | ||
| Self::Deepseek, | ||
| Self::NvidiaNim, | ||
| Self::Openai, | ||
| Self::Atlascloud, | ||
| Self::WanjieArk, | ||
| Self::Volcengine, | ||
| Self::Openrouter, | ||
| Self::XiaomiMimo, | ||
| Self::Novita, | ||
| Self::Fireworks, | ||
| Self::Siliconflow, | ||
| Self::SiliconflowCN, | ||
| Self::Arcee, | ||
| Self::Moonshot, | ||
| Self::Sglang, | ||
| Self::Vllm, | ||
| Self::Ollama, | ||
| Self::Huggingface, | ||
| ] | ||
| } |
There was a problem hiding this comment.
ProviderKind::all() is a manually maintained list with no compile-time guarantee that it matches the enum's variants. If a new variant is added to ProviderKind but forgotten here, it will silently disappear from error-message hints. Consider using a crate like strum with #[derive(EnumIter)], or at minimum a compile-time const array alongside the enum so the mismatch is caught at build time.
| #[must_use] | |
| pub fn all() -> &'static [Self] { | |
| &[ | |
| Self::Deepseek, | |
| Self::NvidiaNim, | |
| Self::Openai, | |
| Self::Atlascloud, | |
| Self::WanjieArk, | |
| Self::Volcengine, | |
| Self::Openrouter, | |
| Self::XiaomiMimo, | |
| Self::Novita, | |
| Self::Fireworks, | |
| Self::Siliconflow, | |
| Self::SiliconflowCN, | |
| Self::Arcee, | |
| Self::Moonshot, | |
| Self::Sglang, | |
| Self::Vllm, | |
| Self::Ollama, | |
| Self::Huggingface, | |
| ] | |
| } | |
| // NOTE: Keep this list in sync with the ProviderKind enum. There is no | |
| // compile-time check; adding a variant without updating this array will | |
| // silently omit it from error-message hints produced by names_hint(). | |
| #[must_use] | |
| pub fn all() -> &'static [Self] { | |
| &[ | |
| Self::Deepseek, | |
| Self::NvidiaNim, | |
| Self::Openai, | |
| Self::Atlascloud, | |
| Self::WanjieArk, | |
| Self::Volcengine, | |
| Self::Openrouter, | |
| Self::XiaomiMimo, | |
| Self::Novita, | |
| Self::Fireworks, | |
| Self::Siliconflow, | |
| Self::SiliconflowCN, | |
| Self::Arcee, | |
| Self::Moonshot, | |
| Self::Sglang, | |
| Self::Vllm, | |
| Self::Ollama, | |
| Self::Huggingface, | |
| ] | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
@mvanhorn — still wanted; it's conflicting after this week's merges. Could you rebase onto current main? Low-risk docs/tests alignment, we'll merge promptly once green. |
2 participants
Summary
Aligns the Hugging Face provider surface with the shipped route, per the polish list in #2706: documented env aliases now actually resolve (
HUGGINGFACE_API_KEYprimary,HF_TOKENfallback, blank-primary handled so the fallback still fires), provider errors listhuggingfaceamong known providers,config.example.tomlanddocs/PROVIDERS.md/docs/MODEL_LAB.mdmatch the code, and a newscripts/check-provider-registry.pydrift check keeps the registry, docs, and example config from diverging again. TUI env handling incrates/tui/src/config.rsmirrors the config-crate resolution so both paths agree.Testing
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-featurescargo test --workspace --all-features(one unrelated TUI test flaked in a single run and passed on re-run; the huggingface tests passed in every run)Checklist
Fixes #2706
Greptile Summary
This PR aligns the Hugging Face provider surface — env var resolution, error messages, docs, example config, and the drift-check script — with the route that was already shipped. The code changes are small and targeted: two new env-var fallbacks in the TUI path (
HF_BASE_URL,HF_MODEL), a newenv_api_key_for_providerhelper in the config crate that correctly prioritisesHUGGINGFACE_API_KEYoverHF_TOKEN(including blank-primary handling), and an expandedcheck-provider-registry.pythat keeps aliases and env-var order in sync across both crates and docs.ProviderKind::all()/names_hint()for richer "unknown provider" errors, andenv_api_key_for_provider()so theHUGGINGFACE_API_KEY→HF_TOKENfallback chain fires in both the "skip secret store" and "secret store returned nothing" paths.apply_env_overrideswithHF_BASE_URLandHF_MODELfallbacks to mirror the config-crate resolution; five new tests cover aliases, error text, short-name fallbacks, and combined-env precedence.PROVIDERS.md,MODEL_LAB.md, andconfig.example.tomlnow document all four aliases, both base-URL env names, both model env names, and correctly place the HF inference-providers route in the "Implemented Today" section.Confidence Score: 4/5
Safe to merge; all three resolution paths (config crate skip-secret-store, config crate secret-store fallback, TUI deepseek_api_key) now agree on HUGGINGFACE_API_KEY → HF_TOKEN priority, blank-primary handling is correct, and the new tests exercise the key scenarios end-to-end.
The functional changes are small and well-tested. The one structural concern is that ProviderKind::all() is a manually maintained list — a future enum variant added without updating it would silently disappear from error-message hints without any compiler warning. The drift-check script also has a first-occurrence limitation that could let an out-of-order resolution path go undetected. Neither issue affects current correctness.
crates/config/src/lib.rs — specifically the new ProviderKind::all() list, which must be kept in sync with the enum manually.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[User sets provider to huggingface / hf / hugging-face / hugging_face] --> B[ProviderKind::parse / ApiProvider::parse] B --> C{Recognized?} C -- No --> D[Error: unknown provider expected deepseek ... huggingface] C -- Yes --> E[Resolve API key] E --> F{CLI flag set?} F -- Yes --> G[Use CLI key] F -- No --> H{Config file has api_key?} H -- Yes --> I[Use config file key] H -- No --> J{Skip secret store?} J -- Yes --> K[env_api_key_for_provider] J -- No --> L[secrets.resolve_with_source huggingface] L -- Found --> M[Return value] L -- None --> K K --> N{HUGGINGFACE_API_KEY set and non-blank?} N -- Yes --> O[Use HUGGINGFACE_API_KEY] N -- No --> P{HF_TOKEN set and non-blank?} P -- Yes --> Q[Use HF_TOKEN] P -- No --> R[Error: Hugging Face API key not found] subgraph TUI apply_env_overrides S[HUGGINGFACE_BASE_URL set?] -->|Yes| T[Use HUGGINGFACE_BASE_URL] S -->|No| U[HF_BASE_URL set?] U -->|Yes| V[Use HF_BASE_URL] U -->|No| W[Use default router.huggingface.co/v1] X[HUGGINGFACE_MODEL set?] -->|Yes| Y[Use HUGGINGFACE_MODEL] X -->|No| Z[HF_MODEL set?] Z -->|Yes| AA[Use HF_MODEL] Z -->|No| AB[Use default model] endComments Outside Diff (1)
scripts/check-provider-registry.py, line 682-696 (link)report_string_orderanchors on the first occurrence of each needle viastr.find(). Incrates/tui/src/config.rsthe correct-priority occurrence ofstd::env::var("HUGGINGFACE_API_KEY")(line ~2506) andstd::env::var("HF_TOKEN")(line ~2511) appear before any of theHF_TOKEN-only "is auth present?" checks later in the file. So the ordering check passes today. However, if a new resolution code path is added that consultsHF_TOKENbeforeHUGGINGFACE_API_KEY, the check would still pass becausefind()returns the first (correctly ordered) site and ignores the new one. Tightening the check to scan within the specific resolution function would make the drift guard more reliable.Reviews (1): Last reviewed commit: "fix: address self-review findings" | Re-trigger Greptile