Repository navigation
Fix: Add IBM Bob's rate to configs written before the preset had it - #1219
Conversation
The built-in config has priced IBM Bob since rossoctl#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 rossoctl#1216 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.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 51 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 (6)
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.
The fix is well-targeted and the reasoning in the PR body holds up under checking.
Notably, the one item listed as "Not verified" — that adding pricing needs no proxy restart — does check out on reading: reloader.New(..., WithOnCommit(applyPricing)) applies the rebuilt table after commit, currency included (unit: is carried inside the swapped Table, not read separately), and migrateBobPricing correctly leaves configChanged alone so an otherwise-current re-install still short-circuits the restart.
I also checked a hazard the PR doesn't mention: it drops two new files (config.yaml.before-agentop-pricing, config.yaml.tmp) into the directory the reloader watches. That's safe — the watcher filters on exact filepath.Base equality with no prefix matching anywhere in core/reloader/reloader.go, so neither sibling arms the debounce timer, and reloadOnce always reads configPath rather than the event path.
Insertion logic is sound in all six YAML shapes: positions come from yaml.v3 nodes and align to the file's existing indentation rather than assuming two spaces. Every failure path fails closed — flow style, non-mapping, unparseable, and failed-verify all leave the original intact with no temp file behind. I confirmed that includes some odd cases the tests don't cover (pricing: ~, a quoted "pricing": key, tab-indented keys); each falls through to an "add by hand" error or a load failure rather than corrupting the file.
Also verified: bobEndpointLines is byte-identical to the built-in block at cmd/cortex/local.go:253-260, and bobPriced's catch-all-multiplier distinction is real — Table.Resolve early-returns ProvNone before any multiplier is consulted, so a multiplier can never manufacture a resolution out of nothing.
Two non-blocking notes inline.
Not verified, matching the PR's own disclosure: no live proxy was exercised. My confirmation of the no-restart path is from reading the reloader and applyPricing, same as the author's — code reading can't rule out a runtime surprise in pricingRegistry.Swap under live traffic.
Summary
Author: huang195 (MEMBER — maintainer)
Areas reviewed: Go (migration logic, service install wiring), Tests, Docs
Agent/IDE config (.claude/.vscode): none
Commits: 1 commit, all signed-off: yes
CI status: passing (26 checks; Spellcheck skipped)
Assisted-By: Claude Code
| // priced. Every production tier of Bob's IDE and shell reaches the gateway as | ||
| // premium-ide, and no bundled table names it, so it resolves only if the | ||
| // operator's own config prices it. | ||
| const bobProbeModel = "premium-ide" |
There was a problem hiding this comment.
suggestion — The probe does resolve to ProvNone today, so the behaviour is right, but not quite for the reason stated. Every bundled rate row is Host: "*" (bundled.go), so the host axis offers no protection at all here — the entire guarantee rests on the model axis, where the broadest bundled patterns (*claude-*opus-*, *claude-*sonnet-*, …) each require the literal substring claude-, and modelNameForms only ever strips prefixes/suffixes, never adds text.
So the real invariant is "premium-ide contains no claude-" rather than "no bundled table names it". That's true and stable, but it's the brittle hinge: if Bob's gateway model name ever became something like bob/claude-opus-5-premium-ide, a bundled row would match and bobPriced would return true from bundled data alone — silently skipping this migration forever, which is exactly the failure mode the PR exists to fix.
Worth restating the comment in terms of the model axis, since that's what actually holds it up.
| // operator's own config prices it. | ||
| const bobProbeModel = "premium-ide" | ||
|
|
||
| // bobEndpointLines is the entry, unindented. KEEP IN STEP with the pricing block in |
There was a problem hiding this comment.
nit — These lines and the builtinConfigYAML block are byte-identical today (checked against cmd/cortex/local.go:253-260), and both directions now carry cross-reference comments, which is a real improvement.
But nothing fails if they drift. The migration's test pins 2 Bobcoins/Mtok and TestBuiltinConfig_PricesBobInBobcoins pins it independently, so a future rate change that edits one copy and not the other leaves both suites green while fresh installs and migrated installs price Bob differently — the quietest possible version of this bug.
A test that resolves both through pricing.Build and asserts equal rates would close it. Not worth blocking over for two call sites; flagging in case it's cheap.
mrsabath
left a comment
mrsabath
left a comment
There was a problem hiding this comment.
I verified this against the source rather than taking the description's word for it, including the item the PR itself lists as unverified.
The "no restart" claim holds. pricing.* is absent from validateReloadable's diff list (core/reloader/reloader.go:383-427, which covers only mode, listener.*, cost_ledger.*, session.*), so a pricing-only edit is accepted rather than rejected. applyPricing is wired as the reloader's WithOnCommit hook (cmd/cortex/main.go:536) and swaps the table atomically via Registry.Swap. The reloader watches the config's directory (reloader.go:153), so the os.Rename in replaceConfig is observed. Not setting configChanged is correct, and the placement before the no-op early return in serviceInstall means it still runs on every install.
Byte-identical to the preset. I fetched cmd/cortex/local.go from main via the API: bobEndpointLines matches the built-in block exactly, comments and column alignment included. The cross-copy doc comment added to builtinConfigYAML is the right mitigation for the duplication.
Insertion logic survives cases beyond the test matrix. I wrote throwaway probes for shapes the tests don't cover, and all behaved correctly: trailing comment as last line, trailing multi-line block scalar, quoted "pricing": key, keys ordered after endpoints:, 4-space indentation, no trailing newline, comment-only endpoints:, and composing with the pin migration afterward (both entries land correctly). CRLF files get \n on inserted lines only — cosmetically mixed but valid YAML and parses fine. The yaml.v3 node-position approach is genuinely more robust than block-end scanning.
Tests pass. The migration suite is green, and the untouched pin-migration tests still pass through the refactored replaceConfig. go vet clean, gofmt -l clean.
On the local failures you flagged: I confirmed all 7 (TestRunExec_BeforeFirstStartRunsAndSaysWhatIsLost, TestLaunchdUsable, TestReportSessionInterruption, TestSystemdUsable, TestServiceManagerUsable_Linux, TestServiceStatus_ReportsTheV070ProxyInThePidfile, TestAdoptablePID_PreRenameProxyOnlyBesideOurs) fail identically on origin/main with no PR changes present. Pre-existing environment leakage, not this PR — and CI passes them.
Resolving through pricing.Build rather than grepping the file text is the right call, and the catch-all-multiplier case is a distinction a text search could not have made. Fail-closed verify plus its own backup is appropriate for rewriting an operator's file. 一举两得 (yī jǔ liǎng dé) — one move, two gains: the refactor into replaceConfig makes the new migration safe and leaves the old one better factored.
Areas reviewed: Go (migration logic, service install wiring), tests, docs
Commits: 1 commit, signed-off: yes; imperative, 67 chars, Fixes #1216
CI status: all 27 checks passing
One nit below, no must-fix.
| case isEmptyValue(lines, pk, pv): | ||
| return insertLines(lines, pk.Line, append([]string{pad(pk.Column+1) + "endpoints:"}, | ||
| indented(bobEndpointLines, pk.Column+3)...)), nil | ||
| case pv.Kind != yaml.MappingNode: |
There was a problem hiding this comment.
nit: pricing: ~ (an explicit null) is correctly refused here — isEmptyValue deliberately returns false for it, and appending indented children under an explicit null would not parse, so the fail-closed behaviour is right and the file is left untouched.
But it falls through to this arm and reports pricing: is not a mapping. That is literally true of a null scalar, yet it reads like a type confusion and does not tell the operator what to change. A dedicated arm naming the case would be more actionable, e.g.:
case pv.Kind == yaml.ScalarNode && pv.Tag == "!!null":
return "", fmt.Errorf("pricing: is set to an explicit null; remove the %q so the entry can go beneath it, "+
"or add the %s entry by hand (see docs/pricing.md)", pv.Value, bobHost)Entirely cosmetic — behaviour is already safe.
4 participants
Summary
The built-in config has priced IBM Bob since #1201, but only on a machine that never had a
~/.cortex/config.yaml.writeBuiltinConfignever rewrites an existing file, so an install from before v0.8.0 keeps leaving every Bob request unpriced after it upgrades.agentop service install(run by bothinstall.shandmake dev-install) now adds the same entry to a config that leaves Bob unpriced.Fixes #1216
How it decides and where it writes
pricing.Buildcreates for the proxy, not looked up in the file text. So anything that already prices Bob's host is left alone: its own entry, a glob like*.bob.ibm.com, or a catch-all rate. A catch-all multiplier prices nothing Bob serves, so it doesn't count. The probe model ispremium-ide, the name every production Bob tier reaches the gateway under.pricing:,pricing:withoutendpoints:, empty values, indented and compact lists, and a fully indented document. Flow style (pricing: {…},endpoints: []) is refused with a message saying so, as the listener migration already does.config.yaml.before-agentop-pricing.Why a separate step from the listener pins
It differs from the pins in both of the things
service installcares about:applyPricingvia the reloader's commit hook, which watches the config's directory and handles atomic renames). If this setconfigChanged, a re-run that was otherwise a no-op would restart the proxy and cut every attached session to apply an edit that applies itself.The tmp → load → rename sequence moves out of
migrateConfiginto a sharedreplaceConfig(path, updated, verify). The pin migration's behaviour is unchanged and its tests are untouched.Answers to the issue's open questions
premium-ideon Bob's host. Catch-all rates count; catch-all multipliers don't.service install, the same as the pins. To price Bob differently, edit the entry's rates rather than deleting it; any entry that prices the host is left alone.builtinConfigYAML's doc comment now names both copies, so the next preset addition is easier to spot.Tests
cmd_config_migrate_pricing_test.goresolves throughpricing.Build, the same asTestBuiltinConfig_PricesBobInBobcoins. Each case checks that Bob's three models price at 2 per Mtok on every tier inBobcoins, and that Anthropic stays bundled in USD. It covers the realistic upgrade (pins present, nopricing:), six YAML shapes, three ways Bob is already priced (file byte-identical, no backup), a catch-all multiplier, idempotency, flow-style refusal, an unparseable config, andreplaceConfig's verify path.Mutation-checked, each turning tests red: probe always reports unpriced,
unit:dropped, rate changed to 3, compact-list column wrong, document indentation ignored, flow check removed, verify skipped.Also run against copies of a real
~/.cortex/config.yamlfrom before #1201. It appended the entry, priced Bob, and a second run was a no-op. The current hand-edited config, which already has the block, came out byte-identical.GOWORK=off go vetandgo test ./...pass forcmd/agentopandcmd/cortex, andgofmt -lis clean. One note for local runs:TestRunExec_BeforeFirstStartRunsAndSaysWhatIsLostfails in a shell that hasSSL_CERT_FILEset by an installed Cortex, and passes with it unset. That's environment leakage and is unrelated to this change.Not verified
The no-restart path was not run against a live proxy. That proxy is shared by every session on the machine. The claim rests on reading the reloader and
applyPricing.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com