Use declarative provider pricing for session cost - #10160
Conversation
![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: e429eb187d
ℹ️ 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".
| registry.register_with_name::<OpenAiProviderDef, _>( | ||
| &config, | ||
| ProviderType::Declarative, | ||
| |_| unreachable!("constructor is not used by this test"), | ||
| ); |
There was a problem hiding this comment.
In this added test, register_with_name is called with only the config, provider type, and constructor, but the method in this file requires the supports_inventory_refresh boolean and an inventory_identity resolver as well. Any cargo test -p goose --lib build that compiles this test module fails before running the test; pass the missing arguments as the existing register_with_name_and_inventory_configured test above does.
Useful? React with 👍 / 👎.
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
Thanks for tackling this, and for splitting it into focused pieces. A few things before this can land:
1. The design: I'd push back on the session-persistence half of this PR. The new accumulated_cost_currency column, the schema migration (14→15), the update-builder plumbing and the import path all exist to feed a single Cost::new(amount, currency) call in the ACP build_usage_updates. Meanwhile the desktop (CostTracker.tsx) never reads the session currency — it gets its currency from fetchCanonicalModelInfo(provider, model), i.e. the pricing/config API and the model metadata directly. So the ACP builder can derive the currency the same way the desktop already does (from the resolved model config / provider registry) at the point it builds the update, without a persisted column and without a migration. Can we drop the schema change entirely and compute the currency where the Cost is built?
2. Bigger picture: we want to move toward tracking cost per message, not per session. Accumulating a single per-session cost (and now a per-session currency) is a model we're trying to move away from — a session can span multiple models/providers with different currencies, and a single accumulator can't represent that. Rather than deepening the per-session accumulation with more fields, I'd rather we keep cost attached to individual messages/usage events. That's another reason not to add the session-level currency column here.
3. The test doesn't compile (also flagged by the codex review, still unaddressed). register_with_name takes 5 args (config, provider_type, supports_inventory_refresh, constructor, inventory_identity) but the new test calls it with the wrong arity/turbofish. The PR description lists this test as having been run — please make sure it actually compiles and passes.
4. Simplify the cost math — see inline.
See inline comments for 3 and 4.
|
|
||
| return Some(CostEstimate { | ||
| amount: (uncached_input_tokens + cache_read_tokens + cache_write_tokens) | ||
| * input_price |
There was a problem hiding this comment.
This subtracts the cache tokens and then adds them straight back: (uncached_input_tokens + cache_read_tokens + cache_write_tokens) is just input_tokens (clamped). So the whole cache read/write dance is dead arithmetic — it reduces to input_tokens * input_price + output_tokens * output_price. Either drop the cache variables and write it plainly, or, if the intent was to price cache reads/writes differently, actually apply distinct rates. As written it's misleading.
|
|
||
| let mut registry = ProviderRegistry::new(None); | ||
| registry.register_with_name::<OpenAiProviderDef, _>( | ||
| &config, |
There was a problem hiding this comment.
This won't compile. register_with_name has signature (config, provider_type, supports_inventory_refresh, constructor, inventory_identity) with 3 generic params <P, F, G> — here it's called with the wrong turbofish and argument count, so cargo test -p goose --lib fails to build this module. codex flagged this too. The description says this test was run; please make sure it actually compiles and passes before we look again.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 348a87b85c
ℹ️ 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".
| let accumulated_cost = cost_estimate | ||
| .as_ref() | ||
| .map(|estimate| session.accumulated_cost.unwrap_or(0.0) + estimate.amount) | ||
| .or(session.accumulated_cost); |
There was a problem hiding this comment.
Because accumulated_cost is session-wide, this adds the new estimate amount to any prior amount without considering estimate.currency; ACP allows changing the model/provider mid-session, and custom declarative providers can declare non-USD currencies. A session that accrues cost on a RUB-priced declarative model and then switches to a USD model will store RUB+USD as a single float, and build_usage_updates will label the whole total with the current model's currency, so clients display a corrupted cost. Track/store the accumulated currency or stop accumulating when it changes before adding these amounts.
Useful? React with 👍 / 👎.
5b80010 to
239bbad
Compare
239bbad to
8f00783
Compare
|
This PR no longer tries to make Instead, the backend persists and emits the durable inputs needed for pricing:
Those values are stored in session extension data as a provider/model usage snapshot and sent to Desktop through optional ACP metadata: usage_update._meta.goose.providerUsage The standard usage_update shape is unchanged. Existing clients can ignore this metadata. Desktop is then responsible for turning usage into displayed cost. It fetches model info for each provider/model, uses provider-declared pricing when available, falls back to canonical pricing when needed, and groups totals by currency. So the responsibility split is:
This keeps the persisted data more stable than storing precomputed costs. If provider pricing changes later, Desktop can re-evaluate the displayed cost from the saved usage snapshot instead of trusting an old accumulated amount. It also keeps backward compatibility:
The existing acpGetCanonicalModelInfo method name is kept to avoid protocol churn, but internally it now behaves as provider-aware model info: provider model info first, canonical fallback second. A later cleanup can rename that API once there is agreement on the protocol shape. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f00783635
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b59bc8a8cd
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c83b7409d
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5100f6ac8
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3edba81dfd
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28e47089a3
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22a82b880a
ℹ️ 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".
|
Thanks for continuing to iterate on this. There are some good ideas here, especially around making cost tracking work better when provider/model pricing is not in the canonical table. Since this PR was opened, #10172 has landed and changed the direction for this area: usage and cost are now tracked through the backend provider-call usage/cost ledger, with session totals derived from that data. #10210 is also building the Desktop UI on top of that path. That means the snapshot-based/session-extension approach in this PR would overlap with, and in places conflict with, the current architecture. I am going to close this PR to avoid asking you to keep rebasing a large change onto a moving target. If there is still a gap around declarative provider pricing fallback, the best next step would be a smaller follow-up PR against current Really appreciate the work here and the thoughtful follow-ups after the earlier review comments. |
2 participants
Summary
Testing
cargo fmtcargo check -p goosecargo test -p goose providers::provider_registry::tests::declarative_provider_cost_uses_declared_model_prices --libcargo test -p goose acp::server::tests::test_build_usage_update --libGUI validation:
input_token_cost,output_token_cost, andcurrency.UsageUpdate.cost.currencymatches the provider/model metadata currency, and falls back toUSDwhen no provider/model currency can be derived.Related Issues
Relates to #9924
Relates to #9992
Screenshots/Demos (for UX changes)
Before:
After: