Repository navigation
fix(cost): exempt BYOK inference from the managed-credit budget - #5218
Conversation
The local [cost] daily/monthly limits exist to cap spend against OpenHuman
credits, but they were enforced against every recorded call. A user routing
all inference through their own key accrued phantom spend and was refused
service at the default $10/day cap for inference OpenHuman never billed:
- BYOK providers echo no charged_amount_usd, so the recording sites price
the call locally from cost::catalog and persist that as spend.
- CostStorage aggregated every record into one daily/monthly figure and
check_budget compared it against the managed-credit limit.
- CostBudgetMiddleware turned Exceeded into "cost budget exceeded: Day
spend ... >= limit ...", which web_errors classifies as budget-exhausted
and renders as "You're out of credits".
Split the aggregates by billing route and gate on the managed subset only.
A pure-BYOK user has zero managed spend, so the gate is now structurally
unreachable for them. Real managed-credit exhaustion is unaffected: it is
enforced server-side and surfaces through the backend's own billing error.
The route is derived from the model id each record already carries (managed
= the backend tier slugs; BYOK is always a real provider id such as
minimax/minimax-m3) rather than stored as a new field. Persisted history
therefore classifies correctly with no migration, which unblocks an
affected user on upgrade instead of making them wait out the window; a
stored field would have had to guess a default for every existing record.
Unknown ids classify as BYOK, so the failure direction is under-enforcing a
local convenience cap rather than over-charging.
BYOK spend is still recorded and still shown in usage totals - the reporter
wanted to understand the counter, not lose it. Only the budget gauge and
the enforcement gate are narrowed to managed spend, since a gauge filling
to 100% against a cap that can never fire is the same phantom limit.
Fixes tinyhumansai#5016
Fixes tinyhumansai#5127
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughManaged and BYOK/local inference are now classified separately. Budget enforcement and dashboard utilization use managed-route costs, while all-route usage remains recorded and visible. Storage maintains both aggregate categories, with tests covering normalization, exemptions, mixed usage, legacy records, and dashboard states. ChangesManaged-route budget enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e3b73137e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
| Filename | Overview |
|---|---|
| src/openhuman/cost/route.rs | New classification module — CostRoute enum, MANAGED_MODEL_SLUGS constant, route_for_model(), and normalize_model_id() loop (order-independent prefix stripping). Six unit tests including a drift-guard against the upstream tier constants. Previous review concern about single-pass prefix ordering is resolved by the loop approach. |
| src/openhuman/cost/tracker.rs | Core change: adds daily_managed_cost_usd/monthly_managed_cost_usd accumulators to CostStorage, threads managed-only classification through rebuild_aggregates and add_record, and switches check_budget and budget_utilization/budget_status to use managed-only totals. Debug logging is now correctly guarded with a tracing::enabled! check. |
| src/openhuman/tinyagents/middleware.rs | Adds an early-return in before_model that skips the budget gate for BYOK/local routes by classifying request.model; handles the mixed-route user case where legitimate managed exhaustion must not block a BYOK in-flight call. |
| src/openhuman/cost/tracker_tests.rs | Seven new integration tests covering: BYOK exempt from daily/monthly limit and warning threshold; BYOK still visible in dashboard totals; mixed managed+BYOK only gates on managed portion; legacy pre-upgrade BYOK records exempt after aggregate rebuild; dashboard budget gauge reflects managed spend only. |
| src/openhuman/cost/mod.rs | Adds pub mod route declaration and re-exports route_for_model and CostRoute at the cost crate level. |
| src/openhuman/config/schema/identity_cost.rs | Doc-comment-only update clarifying that daily_limit_usd and monthly_limit_usd apply to managed inference only. |
Sequence Diagram
sequenceDiagram
participant App as Inference Request
participant MW as CostBudgetMiddleware
participant Router as route_for_model()
participant CT as CostTracker
participant Storage as CostStorage (JSONL)
Note over App,Storage: BYOK / local model path (e.g. "minimax/minimax-m3")
App->>MW: before_model(request)
MW->>Router: route_for_model(request.model)
Router-->>MW: CostRoute::Byok
MW-->>App: Ok() — budget gate skipped entirely
Note over App,Storage: Managed tier path (e.g. "chat-v1")
App->>MW: before_model(request)
MW->>Router: route_for_model(request.model)
Router-->>MW: CostRoute::Managed
MW->>CT: check_budget(0.0)
CT->>Storage: get_aggregated_managed_costs()
Note right of Storage: reads daily_managed_cost_usd<br/>& monthly_managed_cost_usd<br/>(BYOK spend excluded)
Storage-->>CT: (managed_daily_usd, managed_monthly_usd)
CT-->>MW: BudgetCheck::Allowed or Exceeded
MW-->>App: Ok() or LimitExceeded error
Note over App,Storage: Record phase (both routes)
App->>CT: record_usage(usage)
CT->>Storage: add_record(record)
Note right of Storage: all-route totals updated always<br/>managed totals updated only<br/>if route_for_model → Managed
Reviews (3): Last reviewed commit: "fix(cost): exempt the current BYOK reque..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/openhuman/cost/tracker.rs (2)
339-340: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winTwo full scans of
costs.jsonlwhere one would do.
get_monthly_costandget_managed_monthly_costeach take the lock and iterate every record in the append-only store; combined withget_daily_history(7)above,get_dashboardnow makes three full passes. The store has no rotation, so this grows unbounded with workspace age.A single
CostStorage::get_month_costs(year, month) -> (all, managed)accumulating both in onefor_each_recordclosure removes the added pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/cost/tracker.rs` around lines 339 - 340, Replace the separate get_monthly_cost and get_managed_monthly_cost calls in get_dashboard with a single CostStorage::get_month_costs(year, month) operation that scans records once and returns both totals; preserve the existing month-to-date values and error propagation.
78-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the decision, not just the inputs.
The debug line fires before any branch, so a refusal (
Exceeded) orWarningleaves no grep-able record of which limit tripped and on which route. Given this is the exact flow that produced the phantom "out of credits" reports, emitting the outcome makes future reports diagnosable from logs alone.As per coding guidelines, new or changed flows must include verbose, grep-friendly diagnostics covering entry/exit, branches, and state transitions.
♻️ Sketch: log the exceeded/warning branches
let projected_daily = daily_cost + estimated_cost_usd; if projected_daily > self.config.daily_limit_usd { + tracing::debug!( + current_usd = daily_cost, + limit_usd = self.config.daily_limit_usd, + "[cost] budget check exit=exceeded period=day route=managed" + ); return Ok(BudgetCheck::Exceeded {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/cost/tracker.rs` around lines 78 - 88, Update the budget-check flow surrounding the existing tracing::debug! call to emit grep-friendly diagnostics for each decision outcome, including whether the result is exceeded, warning, or allowed, which limit triggered it, and the affected route. Cover the relevant branches and final state transition so logs capture the decision rather than only the cost inputs, while preserving the existing cost calculations and behavior.Source: Coding guidelines
src/openhuman/cost/tracker_tests.rs (1)
250-434: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting the route-exemption tests into their own file.
This addition pushes
tracker_tests.rswell past 584 lines. The BYOK/managed cluster is cohesive enough to live in e.g.tracker_route_tests.rsalongside the shareddated_record/write_raw_recordhelpers.As per coding guidelines, keep files preferably at or below approximately 500 lines and favor small, single-responsibility modules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/cost/tracker_tests.rs` around lines 250 - 434, Move the cohesive BYOK/managed route-exemption tests, including the tests beginning with byok_spend_* and managed_spend_still_gates_when_byok_spend_is_also_present, into a dedicated tracker_route_tests.rs module. Reuse or expose the existing dated_record and write_raw_record helpers there, and update module declarations/imports so the tests compile while tracker_tests.rs remains within the preferred size.Source: Coding guidelines
src/openhuman/cost/route.rs (1)
40-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the tier sync check exhaustive
managed_tier_slugs_stay_in_synconly covers the hand-writtenupstreamarray, so a newMODEL_*_V1constant can be added incrate::openhuman::config::schema::typeswithout tripping this test. Export a single authoritative tier slice fromcrate::openhuman::configand derive the check from that source so adding a tier forces an update here too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/cost/route.rs` around lines 40 - 60, Export a single authoritative slice of all managed tier slugs from crate::openhuman::config, then update managed_tier_slugs_stay_in_sync to compare MANAGED_MODEL_SLUGS against that exported slice rather than a hand-written upstream array. Ensure the authoritative slice includes every MODEL_*_V1 constant so adding a new tier makes the sync test fail until this local billing list is updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/cost/tracker.rs`:
- Around line 333-345: Update the CostDashboard utilization documentation near
budget_utilization to state that it is calculated from managed monthly spend,
not month_to_date_usd. Keep the month_to_date_usd documentation describing the
all-route total and ensure the documented contract matches the
managed_month_to_date_usd calculation.
---
Nitpick comments:
In `@src/openhuman/cost/route.rs`:
- Around line 40-60: Export a single authoritative slice of all managed tier
slugs from crate::openhuman::config, then update managed_tier_slugs_stay_in_sync
to compare MANAGED_MODEL_SLUGS against that exported slice rather than a
hand-written upstream array. Ensure the authoritative slice includes every
MODEL_*_V1 constant so adding a new tier makes the sync test fail until this
local billing list is updated.
In `@src/openhuman/cost/tracker_tests.rs`:
- Around line 250-434: Move the cohesive BYOK/managed route-exemption tests,
including the tests beginning with byok_spend_* and
managed_spend_still_gates_when_byok_spend_is_also_present, into a dedicated
tracker_route_tests.rs module. Reuse or expose the existing dated_record and
write_raw_record helpers there, and update module declarations/imports so the
tests compile while tracker_tests.rs remains within the preferred size.
In `@src/openhuman/cost/tracker.rs`:
- Around line 339-340: Replace the separate get_monthly_cost and
get_managed_monthly_cost calls in get_dashboard with a single
CostStorage::get_month_costs(year, month) operation that scans records once and
returns both totals; preserve the existing month-to-date values and error
propagation.
- Around line 78-88: Update the budget-check flow surrounding the existing
tracing::debug! call to emit grep-friendly diagnostics for each decision
outcome, including whether the result is exceeded, warning, or allowed, which
limit triggered it, and the affected route. Cover the relevant branches and
final state transition so logs capture the decision rather than only the cost
inputs, while preserving the existing cost calculations and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 081e1a1f-4db9-4e5c-bd1e-8f30fd544db0
📒 Files selected for processing (5)
src/openhuman/config/schema/identity_cost.rssrc/openhuman/cost/mod.rssrc/openhuman/cost/route.rssrc/openhuman/cost/tracker.rssrc/openhuman/cost/tracker_tests.rs
…ai#5016) Both BYOK budget tests failed in CI. The implementation is correct — the assertions were over-specified. They called `check_budget(0.01)` against a $0.01 limit, i.e. charging the entire cap to the current request. With BYOK history correctly excluded the managed total is $0.00, so the request is NOT blocked, but projecting the current request's own $0.01 lands at 100% of the limit and trips the 80% warning. The tests then read that `Warning` as a failure. `Warning` explicitly means "request can proceed"; only `Exceeded` gates. So estimate 0.0 (what `CostBudgetMiddleware::before_model` actually passes, and what the sibling warning-threshold test already used) for the `Allowed` assertion, and pin the real guarantee separately: even a request consuming the whole remaining limit is never `Exceeded` because of BYOK history.
|
@coderabbitai review |
✅ Action performedReview finished.
|
…yhumansai#5016) P1 (Codex) — the exemption was only half applied. Excluding BYOK from the managed *totals* still refused a mixed-route user's own-key calls once their managed spend had legitimately crossed the cap: `CostBudgetMiddleware:: before_model` ignored the request and asked `check_budget(0.0)`, which answers from managed totals alone with no idea what route the current call takes. Managed exhaustion therefore disabled the provider OpenHuman never bills for — exactly the bug this issue is about. The middleware now classifies the current request's model via `route_for_model` and skips the gate when OpenHuman is not the biller. P2 (Greptile) — `normalize_model_id` stripped `hint:` then `openhuman/` in a fixed order, so `openhuman/hint:chat-v1` normalized to `hint:chat-v1` and classified as BYOK while `hint:openhuman/chat-v1` classified as Managed. Managed spend could silently stop counting toward the cap depending only on which decoration a recording site applied first. Stripping now loops until neither prefix applies, making the two order-independent, with tests pinning both orders in both directions. P2 (Greptile) — the all-routes aggregate was computed on every budget check purely to populate a debug log (tracing evaluates fields eagerly). It is now behind a `tracing::enabled!` guard.
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
[cost]daily/monthly limits now apply to managed (OpenHuman-credit) inference only. Bring-your-own-key and local inference is still recorded and still shown in usage totals, but can no longer refuse a request.budget_utilization/budget_status) also narrows to managed spend;month_to_date_usd,period_total_usdandby_modelstay all-route totals.Problem
Reported by two users with no OpenHuman credits configured, routing all inference through their own keys (OpenRouter, plus a self-hosted OpenAI-compatible gateway). Both were refused service at a
$10/daylimit they never set, for inference OpenHuman never billed them for.The chain, traced end to end:
CostConfigdefaults toenabled: true,daily_limit_usd: 10.00,monthly_limit_usd: 100.00(config/schema/identity_cost.rs) — enforcement is on by default with no UI toggle, which is why BYOK configured (OpenRouter + OmniRoute fallback), but token usage counter keeps increasing with a phantom $10/day limit #5016 could not find where the limit was configured.charged_amount_usd, so the recording sites price the call locally fromcost::catalog(agent/harness/subagent_runner/ops/graph.rs,tinyagents/observability.rs) and persist that estimate as spend.CostStorageaggregated every record into a single daily/monthly figure, andCostTracker::check_budgetcompared that figure against the managed-credit limit.CostBudgetMiddleware::before_modelturnedBudgetCheck::ExceededintoLimitExceeded("cost budget exceeded: Day spend $10.0136 ≥ limit $10.0000")— the exact string in You're out of credits #5127's screenshot.web_chat::web_errorsmatches that onr"budget.*exceed"and renders "You're out of credits, so I can't run the managed (cloud) model right now."So the counter, the phantom limit, and the refusal message in both reports are one bug: a managed-credit cap applied to spend that was never charged.
Solution
New
src/openhuman/cost/route.rs—CostRoute::{Managed, Byok}androute_for_model().CostStoragekeeps its existing all-route aggregates for reporting and adds a managed-only subset;check_budgetreads the managed subset. Everything else about recording is unchanged.Why the route is derived rather than stored. Managed inference is addressed by the backend's own tier slugs (
chat-v1,agentic-v1, …); a BYOK provider is always addressed by a real model id (minimax/minimax-m3,llama3:8b), because the tier vocabulary only means anything to the managed backend. Every recording site and every persisted record already carries the model id, so:A stored
routefield would have had to pick a serde default for every pre-existing record, and both options are bad: defaulting to Managed leaves affected users blocked for up to a month on the monthly limit — i.e. it would not actually fix the reported incident on upgrade — while defaulting to BYOK silently discards real managed history. Deriving from data that already exists sidesteps the choice entirely.legacy_byok_records_are_exempt_after_an_aggregate_rebuildpins this.Failure direction. Unknown model ids classify as
Byok. The consequence of a wrong guess is under-enforcing a local convenience cap, never over-charging — actual credit exhaustion is enforced by the backend. Matching is exact after normalising case/whitespace and strippinghint:/openhuman/prefixes;a_byok_model_merely_containing_a_tier_name_is_not_managedguards against a substring match quietly re-introducing the bug.Budget gauge.
budget_utilizationandbudget_statusdescribe a limit that now only gates managed spend, so driving them off the all-route total would leave a BYOK user's gauge pinned at 100% against a cap that can never fire — the same phantom limit, just rendered. They now use managed month-to-date. Spend figures themselves stay all-route: #5016 explicitly wanted to understand his usage, not lose sight of it.Out of scope: the billing/credits system itself, backend credit accounting, and any change to how BYOK spend is estimated.
Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/ci-lite.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge. — every changed line is exercised by the 13 tests below; the local suites were not run (see "Validation Blocked"), so the CI coverage lane is the authority here.docs/TEST-COVERAGE-MATRIX.mdreflect this change (orN/A: behaviour-only change) —N/A: behaviour-only change; no feature added, removed or renamed, and the matrix has no cost/budget row.## Related—N/A: the matrix contains no cost/budget feature ID; stated under## Relatedrather than inventing one.TempDirJSONL store; no network at all.docs/RELEASE-MANUAL-SMOKE.md) —N/A: no new surface; this removes an incorrect refusal on an existing path.Closes #NNNin the## Relatedsection — uses the equivalentFixes #5016/Fixes #5127.Impact
budget_exceeded_daily_limit/budget_monthly_exceededstill pass, retargeted to a managed tier id.managed_spend_still_gates_when_byok_spend_is_also_present.costs.jsonlis read and written exactly as before.get_managed_cost_for_monthadds one scan on the dashboard path only, matching the existingget_cost_for_month. No secrets or PII in the new debug log (USD figures only).Related
cost budget exceededstring)[cost]limits have no Settings UI, which is why both reporters could not find or adjust them. Worth its own issue.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/GH-5016-byok-budget-exemption0e3b73137Validation Run
pnpm --filter openhuman-app format:check—N/A: no files under app/ changed.pnpm typecheck—N/A: no TypeScript changed.cost/route.rs(tier slugs classify managed; drift guard against the backend tier constants; the reported BYOK/local model shapes classify BYOK; case/whitespace/hint:/openhuman/normalisation; substring near-misses stay BYOK; only managed counts) and 7 incost/tracker_tests.rs(BYOK exempt from the daily limit, the monthly limit and the warning threshold; BYOK still recorded for the dashboard; mixed managed+BYOK gates only on the managed portion; legacy pre-upgrade BYOK history exempt after an aggregate rebuild; dashboard gauge managed-only while spend totals stay complete).cargo fmt --checkclean.cargo check --libwas clean over the classifier, storage fields, aggregate rebuild and thecheck_budgetswitch; the later hunks were not re-compiled locally — see below.N/A: no files under app/src-tauri/ changed.Validation Blocked
command:cargo test --lib cost::/cargo check --testserror:not attempted — local compile and test runs were explicitly disallowed on this machine part-way through the change (shared-host load), so verification was stopped and handed to CI.impact:Moderate and specific, stated plainly.cargo check --libpassed over the bulk of the non-test change, and onecargo test --lib cost::run that was already in flight reported exactly one error — a private-module import inroute.rs's test module — which means everything else in the lib and test tree compiled. That import was corrected tocrate::openhuman::config::{…}, the pathinference/provider/factory.rsalready uses. Edits made after that point (the corrected import, two smallroute.rssimplifications, doc comments inidentity_cost.rs) have not been compiled.cargo fmt --checkpasses, so every file is at least syntactically valid. If a lane fails, it will be one of these; I will fix on the PR.Behavior Changes
[cost]daily/monthly budget gates managed (OpenHuman-credit) inference only; BYOK and local inference is recorded but never gated.Parity Contract
get_summary,get_monthly_cost,get_daily_history,month_to_date_usd,period_total_usd,by_model) are byte-for-byte unchanged;cost.enabled = falsestill short-circuits enforcement first; recording behaviour and the persisted JSONL format are untouched.Byok(under-enforce a local cap rather than over-charge); legacy records classify from the model id they already store, so no migration and no default-guessing;managed_tier_slugs_stay_in_syncfails if a backend tier is added upstream without being classified, so managed spend cannot silently stop counting; server-side credit enforcement is not touched by any of this.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes