Skip to content

Port Hugging Face billing usage - #567

Open
Finesssee wants to merge 1 commit into
codex/port-0.61.0-provider-detailsfrom
codex/port-0.61.0-huggingface
Open

Finesssee wants to merge 1 commit into
codex/port-0.61.0-provider-detailsfrom
codex/port-0.61.0-huggingface

Conversation

@Finesssee

Copy link
Copy Markdown
Collaborator

Summary

  • Port the Hugging Face billing provider from upstream 0.61.0.
  • Use the existing SourceMode::OAuth lane for explicit API-token transport without adding a global source mode.
  • Support deterministic token-account, configured key, environment, and Hugging Face token-file precedence.
  • Report authoritative billing spend and optional ZeroGPU and identity details without inventing quota windows or reset times.
  • Register the provider across Rust, Tauri, settings, token accounts, frontend catalog, and CLI/dashboard icons.

Validation

  • PASS: Hugging Face focused Rust tests (14 passed).
  • PASS: settings provider catalog test.
  • PASS: Tauri Hugging Face source-routing test.
  • PASS: Tauri commands::tests (94 passed).
  • PASS: Rust and Tauri clippy with -D warnings.
  • PASS: cargo fmt --all and git diff --check.
  • Known unrelated baseline failure: the full Rust suite reproduced one existing failure in cost_scanner::codex::tests::reasoning_survives_scan_rebuild_and_cache_reload (missing key); 1,939 tests passed, 1 failed, 1 ignored. This PR does not touch cost_scanner.
  • Frontend Vitest was not run because apps/desktop-tauri/node_modules is absent; dependencies were not installed to preserve local storage.

This PR is based on the 0.61.0 provider-details carrier branch (PR #564) and is intentionally standalone for review.

@coderabbitai

coderabbitai Bot commented Sep 19, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 6852ad05-91b9-4312-8def-3bc7af328a74

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Finesssee

Copy link
Copy Markdown
Collaborator Author

Thermo-Nuclear Review: PR #567 — Port Hugging Face billing usage

Verdict: REQUEST CHANGES

Structural regressions

  1. Dead constant shipped in the PR: ENV_KEYS is defined and never used. rust/src/providers/huggingface/mod.rs declares const ENV_KEYS: &[&str] = &["CODEXBAR_HUGGINGFACE_API_KEY", "HF_TOKEN", "HUGGING_FACE_HUB_TOKEN"]; but the token precedence is implemented by the private TokenEnvironment struct, which reads the same three env vars by hand via std::env::var(...) in from_process(). ENV_KEYS has exactly one occurrence in the module — its definition. This is exactly the drift hazard the skill calls out: the canonical-looking name suggests one source of truth while the real precedence lives in struct field order (config_api_key → hf_token → hub_token). Either delete ENV_KEYS or make TokenEnvironment::from_process consume it; as written it's a lie waiting to go stale.

  2. clean_token strips quotes by byte-slicing with value[1..value.len() - 1]. If a token starts with " but its trimmed end is a multi-byte character boundary mismatch... actually the guard requires both starts_with('"') && ends_with('"'), so slicing is byte-safe for UTF-8. But the quote-stripping behavior itself is suspect: a Hugging Face token that legitimately begins and ends with quote characters (tokens are hf_-prefixed, so practically impossible) would be silently mangled. More importantly this heuristic — trim, strip one layer of quotes, trim again — is bespoke string-shaping logic that belongs next to the other credential normalizers, not inline here. Minor, but it's the kind of magic that hides assumptions.

  3. Third provider in the series with its own private bounded-body reader. Port Muse Code subscription usage #568 and Port Nous Portal subscription credits #569 each carry a read_bounded_body/append_bounded_body (streaming chunk accumulation with a 512 KiB cap); this PR reads with response.bytes() and checks body.len() after full buffering — so a hostile server can still stream an unbounded body into memory before the size check fires. The cap is enforced post-hoc, defeating the point the sibling PRs (correctly) implemented incrementally. This is a real inconsistency in the series: Port Hugging Face billing usage #567 is the least defensive of the three buffered variants while claiming the same MAX_RESPONSE_BYTES bound. Fix by reusing the shared streaming reader the other PRs should be contributing.

Missed simplification opportunities (code-judo)

  1. TokenEnvironment is six Option<String/PathBuf> fields + a 4-step file-candidate chain for "find first existing token source." The struct exists purely to make from_process() testable — a good instinct — but resolve()'s two-phase logic (env candidates, then file_candidates().into_iter().find_map(...)) plus push_unique dedup closure could be a single ordered iterator: [env candidates..., file candidates...] with .find_map. The push_unique closure exists only to deduplicate paths that provably can't collide given distinct parents (only the default-cache path can equal an XDG-derived one in exotic setups). The whole dedup + 6-field struct is ~60 lines for a 4-candidate precedence walk; a slice of (Option<String>, fn(PathBuf) -> PathBuf) closures or a flat list would halve it.

  2. build_result is 90 lines of sequential if let Some detail assembly — six display-detail blocks plus cost snapshot plus the three-way identity sub-block. Same shape as Port CodeRabbit CLI usage #566's fetch_result. The pairs-of-(id, title, Option) fold would compress this to ~20 lines. This is the third such projection function in the series; it should be one shared helper.

  3. parse_timestamp tries i64, then u64, then RFC 3339 string — the u64 arm is unreachable in practice (as_i64 accepts anything as_u64 would, since serde numbers that fit u64 fit i64 only if positive — negative u64 is impossible from JSON, and huge u64 fails both). The u64i64::try_from arm is defensive noise; collapse to as_i64 + string arm.

Spaghetti / branching complexity

  • classify_status is a clean 5-arm status ladder — good shape, shared with Port Muse Code subscription usage #568's status_error and Port Nous Portal subscription credits #569's status_error (three near-identical copies: UNAUTHORIZED/FORBIDDEN→AuthRequired (HF maps FORBIDDEN to Other instead — an intentional divergence but unexplained), 429→Other, 5xx→Other, fallthrough→Other with status). HF's FORBIDDEN→Other("token cannot access billing data") vs. siblings' FORBIDDEN→AuthRequired is a real behavioral divergence across the series with no comment explaining why. If intentional (HF billing scopes vs. auth), document it; if drift, unify.
  • parse_identity's (name.is_some() || email.is_some() || plan.is_some()).then_some(...) is fine.

Boundary / abstraction / type problems

  1. SourceMode::OAuth is being used as a generic "token/API lane" with an inline comment admitting the pun: "The shared source enum uses OAuth as the persisted token/API lane for providers whose transport is not an OAuth flow." That comment in fetch_usage is a boundary confession — HF has no OAuth flow; the PR reuses the lane to avoid adding a global source mode. Acceptable as a series-local convention (Port Nous Portal subscription credits #569 does the same), but then the convention belongs on SourceMode's docs, not buried in one provider's match arm. Three PRs (Port Hugging Face billing usage #567, Port Muse Code subscription usage #568, Port Nous Portal subscription credits #569) each re-explain the same pun in their own words.

  2. MAX_SAFE_INTEGER as a filter on numRequests (*value <= 9_007_199_254_740_991) — a u64 JSON number above 2^53 can't be represented in f64 anyway, and as_u64 already gates type; the constant is dead-accurate but the check reads like cargo-culted JS interop. One comment or deletion.

  3. Identity details (account-name, account-email, account-plan) flow through display_details rather than the structured account_email/plan_name bridge fields that Port CodeRabbit CLI usage #566's and Port Venice web subscription credits #565's tests assert (snapshot.account_email, None). Here HF has the email and deliberately puts it in a display detail row instead. The AGENTS.md rule "never show identity/plan/email from provider A in provider B UI" is satisfied (siloed per provider), but the inconsistency means HF identity renders as generic rows while other providers' identity uses the typed fields. If that's the intended UX for optional identity, fine — but it should be a stated decision, not an accident of porting.

File-size / decomposition concerns

  • New file rust/src/providers/huggingface/mod.rs is 771 lines, ~350 of which are tests. Core logic ~420 lines. Under the 1k bar; no split required, but the TokenEnvironment block (~130 lines) is the cleanest extraction if the file grows (e.g. when the shared token-file helpers move to providers/mod.rs per the series recommendation).
  • settings/api_keys.rs +1 entry and settings/tests.rs +1 line: standard registration, fine.

Lower-priority notes

  • month_start uses .single().expect("valid UTC calendar month start") — an expect in production code, but the invariant (day-1 of a real UTC month) genuinely cannot fail. Acceptable, though unwrap_or_else(now) would avoid the panic path entirely for zero cost.
  • fetch_json wraps the whole request in a second tokio::time::timeout even though the client already has PRIMARY_TIMEOUT — double timeout layers (client + wrapper). The optional-fetch path overrides with 2 s, which is the actual justification; a comment saying so would prevent someone from "fixing" the redundancy.
  • expand_tilde duplicates ~10 lines of the same logic as Port Nous Portal subscription credits #569's expand_home. Same extraction target.

Series note: provider.rs all() count bump 71→72 assumes solo merge (see #566's review). brand_color collision: ProviderId::CodeRabbit => "#FF5C35" in #566 equals the existing Chutes color — harmless visually, but also note #567's HuggingFace => "#FFD21E" vs. the frontend registry's #ffd21e are consistent, fine.

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.

1 participant