Repository navigation
Feat: Price IBM Bob in Bobcoins in the built-in local config - #1201
Conversation
A new install (install.sh or make dev-install, both via authbridge-proxy --local --write-config) left every Bob request unpriced: Bob's models -- premium-ide, router, openai/gpt-oss-20b -- are in no bundled table, so abctl showed "-" for Bob until its user found the worked example in docs/pricing.md and copied it in by hand. The built-in config now carries an endpoint entry for api.us-east.bob.ibm.com with unit Bobcoins and a "*" model at the flat 2 per million tokens Bob bills on every tier. The unit is what keeps those figures out of the dollar totals (rossoctl#1153, rossoctl#1195). A config entry rather than a shipped pricing rule: the rate is not on any vendor list, so it belongs where the user can see and edit it, not hidden in the binary where it would go stale silently. The trade is that an existing ~/.cortex/config.yaml is never rewritten, so existing installs still have to add the block themselves. The test resolves through pricing.Build rather than reading the struct, so a unit or model key the table would not match fails in CI, and pins that the "*" model does not reprice other hosts' traffic in Bobcoins. Signed-off-by: Hai Huang <huang195@gmail.com> Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
docs/pricing.md used Bob as the worked example of unpriced traffic and showed its entry with unit "credits" and a single premium-ide model. The built-in local config now ships the entry, so the example matches it (Bobcoins, "*"), and the gap paragraph says which installs still have the gap: any ~/.cortex/config.yaml written before this, since the installers never rewrite one. Signed-off-by: Hai Huang <huang195@gmail.com> Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 33 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
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. Comment |
esnible
left a comment
esnible
left a comment
There was a problem hiding this comment.
Summary
The live config block and its test are correct on every point I checked: "*" is a real glob (table.go:86, with exact-key-beats-glob ranking), unit: is the right YAML key (config.go:73), free-form Bobcoins passes normaliseUnit and is case-preserved so only USD is canonicalised, and the entry is scoped per-host so it cannot reprice anyone else's traffic. TestBuiltinConfig_PricesBobInBobcoins resolving through pricing.Build rather than reading the struct is the right call, and its negative control on api.anthropic.com holds. The PR body's claim that existing installs are never rewritten is verified with three independent guards (install.sh:1374, Makefile:119, and writeBuiltinConfig itself at local.go:380). pricing: appears exactly once as a live top-level key, so there's no duplicate-key hazard.
One suggestion and one nit below; none blocking. A second nit that GitHub won't let me anchor inline (the line is outside the diff hunks): docs/pricing.md:351 and :364 still use credits in the mixed-unit sample, ~25 lines below a worked example that now establishes Bobcoins as the non-USD unit in this document — a reader may wonder which is Bob's. Neither is wrong; switching that sample pair would just remove the double-take, and 390 is generic enough to leave. (premium-ide at 159 is not stale — it's a model that was unpriced, not a rate-card key, and 165-171 frames it correctly.) The suggestion is a usability regression in a commented example rather than in shipped behaviour, but it's a two-line move and worth doing before merge.
Separately, and not this PR's to fix: config.Load uses plain yaml.Unmarshal with no KnownFields (core/config/config.go:926), so a typo'd key anywhere in this template is silently dropped. The new test covers the pricing block specifically; the rest of the template is unprotected.
Author: huang195 (MEMBER — maintainer)
Areas reviewed: Go, embedded YAML config template, tests, docs
Agent/IDE config (.claude/.vscode): none
Commits: 2 commits, all signed-off: yes
CI status: passing (27 checks)
Assisted-By: Claude Code
| # pricing: | ||
| # endpoints: | ||
| # - hosts: ["my-gateway.example.com"] | ||
| # multiplier: 0.80 # a FRACTION of list, so 0.80 is a 20% discount |
There was a problem hiding this comment.
suggestion — this example is now stranded above the block it belongs to.
The comment at 236-237 says "Add it under endpoints below:", but the snippet sits above the new live pricing: key (251), separated by the Bob paragraph, and it lost the # pricing: / # endpoints: parents it had on main. On main these four lines were self-contained — uncommenting them produced a valid standalone block. Now, uncommenting just these two lines puts a bare sequence item at document root and the config fails to load:
expected '<document start>', but found '<block mapping start>'
line 27, column 1: pricing:
(verified by extracting the template and parsing the result.) Note where that error points: at pricing:, twelve lines below the lines the user actually edited. The diagnostic actively misdirects.
The indent is off by one too — 5 spaces before the dash here vs 4 at line 253 — because the snippet was authored to be read with the # gutter stripped, not uncommented in place.
Suggest moving these two lines to just after 260, inside the live endpoints: list, at 4-space/6-space indent, so uncommenting yields a valid second list item. The prose can stay where it is. A multiplier-only endpoint is legal once correctly placed, so nothing else needs to change.
| t.Errorf("%s on %s: provenance %v, want configured", model, bob, prov) | ||
| } | ||
| // Rates are stored per token; the config states them per million. | ||
| for tier := pricing.TierInput; tier <= pricing.TierOutput; tier++ { |
There was a problem hiding this comment.
nit — this loop is correct today and the "every tier" claim in the doc comment holds: TierOutput is 3, the last of the enum (TierInput=0, TierCacheWrite=1, TierCacheRead=2, TierOutput=3), so 0..3 does cover cache read/write.
But it reads as "input through output," and it would silently stop covering a tier appended after TierOutput. tier < pricing.NumTiers states the intent — which is what the doc comment on NumTiers (rates.go:45) says that constant exists for.
The built-in config has priced IBM Bob since #1201, but writeBuiltinConfig never rewrites an existing ~/.cortex/config.yaml. So every install from before that release keeps leaving each Bob request unpriced, and upgrading does not fix it. agentop service install, which both install.sh and make dev-install run, now adds the same entry when the config leaves Bob unpriced. It decides that by asking the pricing table the proxy builds, not by searching the file, so anything that already prices Bob's host (its own entry, a glob, a catch-all rate) is left alone. A catch-all multiplier prices nothing Bob serves, so it does not count. The entry goes in directly under its parent key, at the file's own indentation, and the result must load and price Bob before it replaces the file. The previous file is kept as config.yaml.before-agentop-pricing. This is a separate step from the listener pins on purpose. Pricing hot-reloads, so adding it does not make install restart the proxy and cut attached sessions. And a config without the entry exposes nothing, so a failure only warns instead of refusing the install. Fixes #1216 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
3 participants
Summary
A new install left every IBM Bob request unpriced. Both
scripts/install.shandmake dev-installwrite the config withauthbridge-proxy --local --write-config, and that built-in config had no Bob entry. Bob's models (premium-ide,router,openai/gpt-oss-20b) are in no bundled table, soabctlshowed—for Bob until the user found the worked example indocs/pricing.mdand copied it in.The built-in local config now ships the entry:
unitkeeps Bob's figures out of the dollar totals (#1153, #1195).Why a config entry, not a shipped rule
The rate is on no vendor list. It belongs where the user can see and edit it, not compiled into the binary where it would go stale without warning if IBM reprices.
What does not change
~/.cortex/config.yaml, so older installs still need to add the block by hand.docs/pricing.mdnow says so."*"model reprices nobody else's traffic.Tests
TestBuiltinConfig_PricesBobInBobcoinsresolves throughpricing.Build, the same path the proxy uses, rather than reading the struct. Bob's three models must price at 2 per Mtok on every tier inBobcoins, andclaude-opus-5-5onapi.anthropic.commust staybundledin USD. Mutation-checked: wideninghoststo"*"and droppinguniteach turn it red.Refs #943
Assisted-By: Claude Code