feat: add experimental CLI to price unpriced AI models - #27926
Conversation
9c98399 to
b137245
Compare
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
d4fb46f to
97f7500
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 21 findings (1 P2, 11 P3, 7 Nit, 2 Note), COMMENT. Review Finding inventoryFinding inventoryFindings
Contested and acknowledged(none yet) Law analysis
Round logRound 1Panel. Reviewed against b721935..97f7500. Netero + Law ran in parallel; Netero returned P3 only so the panel gate opened. 17-reviewer panel (Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Komugi, Gon, Leorio, Chopper, Ging-go, Kite, Knuckle, Kurapika, Luffy, Ryosuke) + wildcards (Meruem, Razor). New: 1 P2, 11 P3, 7 Nit, 2 Note. Komugi, Ging-go, Knuckle, Kurapika returned no findings; Kurapika chained the auth verification and Kite added four praise notes. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid feature end-to-end: RBAC and entitlement gates wire through cleanly, the WritesNothingWhenAnyEntryIsInvalid test proves the batch atomicity claim, and consolidating the provider list into one package retires an old duplication. Test density is 52.5% and the TestValidateAIModelPrices matrix is thorough.
Severity spread: 0 P0, 0 P1, 1 P2, 11 P3, 7 Nit, 2 Note. The P2 is the missing MaxBytesReader on the upsert body, which is the single common pattern used by every other JSON-body endpoint in this repo. The most consequential P3 is the silent one-way overwrite that runs when a later Coder release adds a model to the seed that an operator has already priced; three reviewers converged on it and it is worth naming in the docs and adding a startup warning even before AIGOV-589 lands. PUT on a plural collection URL with partial-update semantics also deserves a decision before the endpoint graduates out of exp.
A note from Kurapika on the chain review: "No TOCTOU. IsDefaultPriced is served from a sync.OnceValue over the compile-time embedded seed. prices.Seed runs once in coderd.New before the router accepts requests; there is no mid-lifecycle re-seed that could race an operator write."
Law ran and returned "Don't split, Advisory." Effective LOC 2151 across four layers all serving the same feature; the only clean vertical carve is the provider list consolidation, which is too small to justify its own PR, and horizontal cuts would split the validation contract between the endpoint and the CLI.
coderd/aibridge/prices/prices.go:39
P3 [CRF-3] The startup seeder silently overwrites operator-set prices when a later Coder release adds that model to the embedded book; no log, no audit, no way to restore. (Mafuuu P3, Pariston P3, Razor P3, Luffy Note)
validateAIModelPricesrefuses a price for any model in the embedded book, "so a price set for a model it covers would not survive a restart."Seedre-applies the embedded book on every startup, and theIS DISTINCT FROMguard inqueries/aicostcontrol.sqlonly skips rewriting when the columns already match. Take an operator who pricesanthropic/footoday because Coder has no price for it, and priced it because their contract is $2/mtok, not the models.dev list price. When the next Coder release addsanthropic/footo the embedded book, restart runs the seeder,IS DISTINCT FROMfires, the row is rewritten to the book's price with a freshupdated_at, and cost reports for that model change.
IsDefaultPricedguards write-time only, so the sequence is: (1) Operator pricesanthropic/new-modelat their negotiated rate. (2) Next Coder release addsanthropic/new-modelto the seed. (3) Operator upgrades.Seed()overwrites their price with the vendor default. No warning, no log line naming the changed rows, no audit event. The startup log fires only on failure.
Orchestrator: three P3s converged on the same lifecycle mismatch. AIGOV-589 tracks the deeper fix (source column so the seeder only touches its own rows). Two cheap mitigations land inside this PR: log at Warn from the seeder when a (provider, model) row already exists with prices that differ from the seed's before overwriting it, and add a paragraph to docs/ai-coder/ai-gateway/cost-controls.md alongside "Prices are not retroactive." Neither replaces the ticket; both let an operator notice.
🤖
coderd/aibridge/prices/prices.go:22
Nit [CRF-14] The "three places" comment is stale; codersdk.AIModelPriceUpsert is a fourth copy of the same JSON contract. (Mafu-san Nit, Meruem Note)
The comment says the seed contract lives in three places (the seeder struct here, the generator's
priceRow, and the SQL extraction). This PR addscodersdk.AIModelPriceUpsertwith the same sixjsontags. The upsert handler doesjson.Marshal(req.Prices)on the typed slice and passes the bytes toUpsertAIModelPrices, so any field this struct does not know about is silently dropped from the wire, unlike the seed path where the raw file bytes flow through to SQL. That makesAIModelPriceUpsertthe strictest gate of the four.
Orchestrator: update the comment here and the mirror at scripts/aibridgepricesgen/main.go:66 to name codersdk.AIModelPriceUpsert as the fourth place, or the next author adding a price column will update three copies and drop the field on the new PUT endpoint. This bites the same target as CRF-9; both are cheap in one edit.
🤖
🤖 This review was automatically generated with Coder Agents.
| @@ -212,7 +212,46 @@ Replace `<VERSION>` with your Coder minor version, for example `2.36`. | |||
| Monitor `coder_ai_gateway_cost_control_unpriced_token_usage_records_total`, | |||
| labeled by `provider` and `model`, to detect unpriced usage. Any non-zero value | |||
| means spend is under-counted. Because the price book ships with the release, a | |||
There was a problem hiding this comment.
Note [CRF-21] The workflow starts with "find models you haven't priced yet" but the CLI has no way to answer that question. (Luffy)
The docs page routes the operator through metrics: monitor
coder_ai_gateway_cost_control_unpriced_token_usage_records_total, read theproviderandmodellabels off it, type them intocoder exp ai-model-prices update. The CLI has no way to answer the questionwhich models has this deployment seen traffic on that don't have a price yet?, which is the only question that leads to using this command at all.
Orchestrator: not this PR's job to build the query. Worth naming as the obvious next step so it doesn't get lost. A list --unpriced (or a subcommand that reads recent interception rows and diffs them against ai_model_prices) would close the loop.
🤖
There was a problem hiding this comment.
This is an interesting idea 🤔 my only concern is that this would process a lot of interceptions (depending on the retention period), meaning this could be a potentially heavy operation. We would need to investigate this better.
434f269 to
ef0ce63
Compare
| // IsDefaultPriced reports whether the embedded price book already carries a | ||
| // price for the model. Coder owns those prices and re-applies them on every | ||
| // startup, so an operator price set for one would not survive a restart. | ||
| func IsDefaultPriced(provider, model string) bool { |
There was a problem hiding this comment.
Initially, I thought we would need to restore the models removed between 2.35 and 2.36. However, I think this is not required. After the patch lands, the CLI will only be available after 2.36, meaning that whatever was released on the price book is what is considered a model with "default" source (once AIGOV-589 lands), meaning that if operators decide to add custom values for removed models, these will be considered "custom".
So we have 2 paths going forward:
- Only consider prices that were available on 2.36 as default, meaning that if an operator changes a model that was available on 2.35 this will be considered a "custom" model.
- Consider all prices from when the prices table was first introduced in 2.35. However, if we do decide this route, we need to restore these models in the patch as well. Otherwise, operators might change these models via the CLI, and then once we introduce the previously removed models and the source column, these models would be considered "default" and would overwrite the prices defined by the operator.
Given that cost control was officially released in 2.36 and the cost reporting via the effective group is only available from 2.36 (even though raw interception cost without a group was released in 2.35, but all the data returned by the endpoints always guarantees the effective group constraint), and that it is unlikely that deprecated models will still be used, I would say option 1 is an acceptable approach.
However, I think this brings up another topic on how we should handle "deprecated" models going forward: should we remove them from the price book? Because this would mean that for customers that skip a version, some models might be considered "unpriced", while for customers that do not skip any versions, they would be considered "default" (since we never delete models from the database). In conclusion, this would mean that we could end up with different customers with different default models in their databases. Wdyt?
There was a problem hiding this comment.
I think it's acceptable for those models.
However, it might be simpler to just keep deprecated models around so that we are more likely to have a consistent list of prices. It's not a huge amount of data.
There was a problem hiding this comment.
Discussed internally and we decided that it is ok to not keep removed models in the price book. With the introduction of custom pricing, customers will be able to introduce models not built-in if needed.
ef0ce63 to
ef37d59
Compare
| // An entry sets all four columns, so an absent key would clear that | ||
| // price rather than leave it alone. |
There was a problem hiding this comment.
We explicitly require all price fields to be included in the input for each model to avoid potential confusion about whether the missing fields are updated to null or kept as in the row. Similarly to the price book always including all fields, with null expressing unknown values.
| _, err = fmt.Fprintln(inv.Stdout, "No changes to apply.") | ||
| return err | ||
| } | ||
| printAIModelPriceChanges(inv, additions, changes) |
There was a problem hiding this comment.
Actual content validation (like price book models, provider as a supported provider, non-negative prices, etc) is done server-side, while on the client side we only do flag validation. Therefore, we show a little plan (similar to terraform plan) before sending the request upstream.
Plan: 1 to change.
~ anthropic/my-model
cache_write_price $0.000005 -> -
> Apply? (yes/no)
But the resultant plan may fail. I would prefer avoiding moving certain validation client side. I think this is acceptable, but let me know what you think
There was a problem hiding this comment.
I think we should avoid client-side validation; it tends to get out of sync.
There was a problem hiding this comment.
I think it's acceptable for those models.
However, it might be simpler to just keep deprecated models around so that we are more likely to have a consistent list of prices. It's not a huge amount of data.
ef37d59 to
2c6d22c
Compare
2 participants
Description
AI Gateway computes the cost of an interception from
ai_model_prices, which is seeded on every server start from a price book embedded in the binary. A model the price book does not cover records a NULL cost, so its spend is invisible to cost reporting and is not enforced against budgets. The only fix was to wait for a Coder release that added the model.This adds an experimental CLI, backed by an experimental HTTP endpoint, for pricing those models. Models the price book already covers are rejected, because the seeder re-applies the book on every start and would overwrite an operator price. Support for custom pricing will be handled in https://linear.app/codercom/issue/AIGOV-589/extend-experimental-cli-command-to-set-custom-ai-model-prices.
Commands
Changes
GETandPOST /api/experimental/ai/model-prices, gated behind the AI Bridge entitlement and the existingai_model_priceRBAC resource.GetAIModelPricesquery with optionalproviderandmodelfilters applied in SQL.prices.IsDefaultPriced.coder exp ai-model-pricescommand withlistandupdate.updateaccepts a JSON document or the single-model flags and prints a plan, asking to confirm unless the document is piped in or--yesis passed.coderd/aibridge/prices/providersso the price generator and the server share one definition.codersdktypes and client methods for both endpoints, and bound the request body at 1 MiB.Closes https://linear.app/codercom/issue/AIGOV-567/experimental-cli-command-to-set-prices-for-unpriced-ai-models
Note
Initially generated by Claude Opus 5, modified and reviewed by @ssncferreira