[amiddavid]

No behavior change. A table test per caller compares every price with a fixture made on origin/main c2a06dd. Data dependence: no change (prices and model ids only).

• Invalid input: a negative cache multiple now fails to load. The owner decided this is not a behavior change, because negative prices do not occur.
• Offline research tool, not product code: deploy/harbor/kv_ttl_cost_model.py now fills a blank cache_write per family, as Go does (1.0 for non-Anthropic families with no write premium, 1.25 for Anthropic and GPT-5.6+). No product code, dashboard or proxy runs this file, and it is not shipped. Only experiment numbers for non-Claude models change. The owner approved it.
• Agent rates stay literals in extract_econ.go and proxy/proxy.go. A test pins each to the new function within 1e-12. Computing them at run time moved 0.3 by 1 ulp. The owner approved it.

Closes #856

Summary

• One function decides a cache price: Price.CacheReadFor, CacheWrite5mFor, CacheWrite1hFor.
• Rule, in order: a stated rate, then the operator's multiple, then the model's multiple.
• Callers: ParseTable, Table.Price, the LiteLLM feed, kvcache.FromPrice, agentRates, HaikuPricing, dash/keepalive.go median ping.

What changed

• internal/modelinfo: the three methods, Resolved, ReadFrac / WriteFrac / Write1hFrac on Price (not in JSON). The readDerived flag is gone.
• LiteLLM feed: a negative stated cache rate is kept as before, so valid and invalid data price the same as on main.
• Ping ratio text, from the page's net formula (1.25 - r) / r: 11.5x at 0.1x, 24x at 0.05x, 49x at 0.025x (dash/ui/app.js, docs/how-to/cache-keepalive.md).
• refactor-scan/lessons.md: "Centralize the decision, not the constant".

How it was tested

• ratefill_test.go in 6 packages. The model list and check helper are shared in internal/modelinfo/ratefilltest. Fixtures come from c2a06dd.
• Go and Python parity test: same price file and models, every field, every family, write tier included.
• Tests for a negative cache_write_frac in Go and Python.
• Box, -race, at the final head: ./internal/..., ./kvcache/..., ./components/offload/..., ./cheapmodel/..., ./deploy/..., full ./dash/..., full ./proxy/... pass. gofmt -l empty, go vet ./... clean, Python test OK.

[amiddavid]

Reviewer	Kind	Started by	Context	Review	Round	Head	Verdict
Reviewer 2 (glm, user of @amiddavid)	GLM rits/zai-org/glm-5-3 via Claude Code 2.1.316	offload-review skill, @amiddavid's session	fresh	2 of 2	1 of 2	05907c872ccb	blocking

Verdict: blocking

Findings

1. dash/ui/app.js:6538 — the lower-tier ping ratios say 25x and 50x; the arithmetic gives 24x and 49x
   • What fails: a user reads the keep-alive settings page (and docs/how-to/cache-keepalive.md:33) and is told a ping buys back "about 25 times" at a 0.05x read rate and "about 50 times" at 0.025x. The page's own formula for the 0.1x anchor is net saving over ping cost: (1.25 − 0.1) / 0.1 = 11.5. The same formula gives (1.25 − 0.05) / 0.05 = 24 and (1.25 − 0.025) / 0.025 = 49. The shipped numbers are the gross write/read ratios (1.25 / 0.05 = 25, 1.25 / 0.025 = 50), so the sentence mixes two different measures: net for 0.1x, gross for the lower tiers. It overstates the saving by about 4% and 2%.
   • Proof: python3 -c "print((1.25-0.05)/0.05, (1.25-0.025)/0.025, (1.25-0.1)/0.1)" → 24.0 49.0 11.5. The PR description itself says "gives about 24x and 49x for the lower tiers", which disagrees with the text it ships.
   • Fix: change "about 25 times" to "about 24 times" and "about 50 times" to "about 49 times" in dash/ui/app.js:6538 and docs/how-to/cache-keepalive.md:33.

Non-blocking

• internal/modelinfo/table.go:118 — a negative file-level cache_write_frac is not validated. ParseTable validates cache_read_frac > 0 but only replaces cache_write_frac == 0 with the default. Before this PR a negative value produced a negative write rate; now WriteFrac > 0 is false, so the value is silently ignored and the per-model default applies. Both old and new behavior are garbage on invalid input; new is less garbage. Suggest validating cache_write_frac > 0 like cache_read_frac. Note the Python port (kv_ttl_cost_model.py:498) also passes a negative through, and the parity test does not cover a negative frac.
• The ratefillModels / ratefillCheck / ratefillF block is copied verbatim into 7 test packages (about 90 lines each). A shared helper in one internal package would keep the model list in one place when a new tier ships. Test-only; acceptable as is.
• deploy/harbor/kv_ttl_price_parity_test.go:95 — the knownWriteDiff branch checks only Go's side (write == input for non-premium families) and skips Python's field-3 value entirely. A Python regression in the write fill for those families would not be caught. The divergence is pinned and named for a human, so this is fine.
• PR open question 1 (the agentCacheReadPerMTok literals stay, to avoid a 1-ulp change) is the right call; the fixture test pins it.

What I checked

• Read the full diff c2a06dd5..HEAD (26 files) and the code around each change: internal/modelinfo/modelinfo.go, table.go, cachefrac.go, litellm/litellm.go, kvcache/pricing.go, components/offload/extract_econ.go, cheapmodel/usage.go, dash/keepalive.go, dash/ui/app.js, docs/how-to/cache-keepalive.md, deploy/harbor/kv_ttl_price_parity_test.go, deploy/harbor/kv_ttl_cost_model.py.
• Proved the refactor is behavior-preserving at each migrated call site, old code vs new:
  • Table.Price: read tier still derives from the QUERIED model when the file sets no cache_read_frac; write tier still derives from the entry's own match family (chargesCacheWritePremium(match)), file multiple wins. The removed readDerived flag is replaced by ReadFrac == 0, same semantics.
  • litellm.fetch: p.Resolved(k) equals the old two if == 0 fills against the same key k.
  • kvcache.FromPrice: caller read multiple wins, unset follows the model; write multiples carry defaults; the 1h branch and the Write5m-ratio fallback are unchanged.
  • agentRates, HaikuPricing, dash/medianPingUSD: stated tier wins, then per-model multiple; empty model keeps the Anthropic constants. Verified each old expression against the new one term by term.
• Traced every reader of Price.CacheRead / CacheWrite outside modelinfo (dash, proxy, keepalive, kvcache, components): all obtain prices through a Pricer (Table or LiteLLM), both of which now resolve, so no reader sees a blank tier. Ctx.SelfRates comes from Handler.selfRates, also Pricer-backed.
• Checked the new ReadFrac / WriteFrac / Write1hFrac fields are json:"-", so no wire-format change; no YAML or round-trip serialization of Price exists.
• Spot-checked the fixtures against hand-computed values (internal/modelinfo, kvcache, components/offload, dash, cheapmodel, litellm testdata/ratefill_before.json): opus-5-5 read 2.5e-7 (0.05x), opus-5 5e-7, gemini write at 1.0x, writefrac: 1.5 honored on Anthropic and ignored on gemini, stated cache_write 1.4 beating the file multiple, haiku 1/5/1.25/0.1, median ping 0.032/0.016/0.008. All match.
• Verified the fixture tests catch dropped or added rows (length check), and that the test helpers they rely on exist and compile by inspection (pythonBinFor, kaExpiry, newKAFixture, kaFixture.db, Filter.TenantAll).
• Checked the harbor parity test against the Python port's actual behavior (from_operator_file, for_model, read re-derivation at lookup, the write-frac divergence) for every id in its list, including [1m] suffixes and the empty model.
• Live tests were not run (not approved for this review); all verification above is by reading.

[amiddavid]

Reviewer	Kind	Started by	Context	Review	Round	Head	Verdict
Reviewer 1 (glm, user of @amiddavid)	GLM rits/zai-org/glm-5-3 via Claude Code 2.1.316	offload-review skill, @amiddavid's session	fresh	1 of 2	1 of 2	05907c872ccb	blocking

Verdict: blocking

Findings

1. internal/modelinfo/table.go:128 — a negative cache_write_frac changes behavior, and the PR says "No behavior change"
   • What fails: an operator file with cache_write_frac: -0.5 on an Anthropic-matching entry. On base c2a06dd, ParseTable filled p.CacheWrite = Input * (-0.5) — a negative rate. Now WriteFrac is -0.5, and CacheWrite5mFor (modelinfo.go:96) only uses it when WriteFrac > 0. So it falls through to CacheWriteFracFor(model) = 1.25. The priced cache-write changes sign and size. ParseTable rejects cache_read_frac <= 0 (table.go:94) but still accepts a negative cache_write_frac, so this input reaches the new path. No fixture covers it — the ratefill docs use 0.2, 0.3, 1.1 and 1.5 only.
   • Proof: base table.go, old lines 129-137: frac := f.CacheWriteFrac; ... p.CacheWrite = p.Input * frac — no sign check on frac. New modelinfo.go:92-99: if p.WriteFrac > 0 { return p.Input * p.WriteFrac }; return p.Input * CacheWriteFracFor(model).
   • Why it matters: the repo rule says a behavior change for an edge case needs a human's yes, named at the top of the PR. The PR claims "No behavior change". This input is one. The old output was also wrong (a negative rate), so the new behavior is defensible — but a human should decide, or the input should be rejected.
   • Fix: validate next to the cache_read_frac check in ParseTable: reject cache_write_frac < 0 (0 keeps its "use the default" meaning). Then every accepted file prices exactly as before, and the claim holds.

Non-blocking

• Same gate change, feed side: a negative stated cache rate in the LiteLLM feed (cache_read_input_token_cost: -x) was kept as-is on base (fill only when == 0); now > 0 replaces it with the model multiple. Implausible feed data; noting for completeness.
• The PR body says "about 24x and 49x" for the lower read tiers; app.js and cache-keepalive.md say "about 25 times" and "about 50 times". The pure ratio is 25 and 50; a ping's own input and output tokens pull it slightly below. Both say "about". Pick one pair of figures so the docs agree with the PR.
• deploy/harbor/kv_ttl_price_parity_test.go:87 computes knownWriteDiff from the QUERIED id, but the Go Table decides the write frac from the ENTRY's match. They agree for every id in this test. A future id that crosses families (a claude-named id hitting a non-Anthropic match) would be skipped wrongly. Test-only; fine to leave.
• I could not run Go on this machine (live tests not approved). I verified parity by hand instead: I recomputed fixture rows in all 7 packages against the new code paths, including from/noinput/set, from/readonly/readset, list/set/claude-opus-5-5, agent/noinput, agent/blank/gemini-3-flash-preview, ping/1.25/claude-fable-5-1, ping/0.4/azure/gpt-5.5, litellm/blank/aws/claude-opus-5-5 and the table/* matrix. Every value matched.

What I checked

• Read CLAUDE.md, docs/design.md, and the full diff (26 files, one commit 05907c8).
• Traced the old fill at each of the 7 converted sites (ParseTable, Table.Price, LiteLLM fetch, kvcache.FromPrice, agentRates, HaikuPricing, medianPingUSD) against the new CacheReadFor / CacheWrite5mFor / CacheWrite1hFor / Resolved rule, for valid inputs: stated rate, operator frac, unset frac, blank tier, blank input, empty model, prefix traps, [1m] suffixes, vendor prefixes, non-Anthropic families, GPT-5.5 vs 5.6. No divergence on valid inputs.
• Checked that ReadFrac / WriteFrac / Write1hFrac are set only inside modelinfo, kvcache.FromPrice (on a copy), dash medianPingUSD (on a local unit price) and tests, and that both Pricer sources (Table, LiteLLM) return Resolved prices — so components.TokenRates, SelfRates and the dash price paths never see a frac field. The new fields carry json:"-", so the wire form is unchanged.
• Checked Zero(), Cost, CostWithCacheWrite1h and the Write1h < Write5m clamp for unchanged semantics.
• Verified the parity-test scaffolding exists and is coherent: pythonBinFor, kaExpiry, newKAFixture, the Python PriceBook.from_operator_file, and the pinned Python/Go write divergence.
• Checked the doc text arithmetic: 1.25/0.05 = 25, 1.25/0.025 = 50.

[amiddavid]

Finding 1 (negative cache_write_frac): fixed in d23257c. ParseTable and the Python reader now reject a negative value. 0 still means "default". Tests added in both. Listed as item 3 in the PR body.

[amiddavid]

Finding 2 (ratio text): fixed in d23257c. Both places use the net formula (1.25 - r) / r: 11.5x, 24x, 49x. The PR body uses the same numbers.

[amiddavid]

LiteLLM negative stated rate: fixed in d23257c. A negative stated cache rate is kept as on main, so no price changes for valid or invalid data.

[amiddavid]

Parity test: the knownWriteDiff branch is gone. The test compares all 5 fields, including the write tier, for every family.

[amiddavid]

Fixture block: moved to internal/modelinfo/ratefilltest. Each package keeps only its own test and fixture.

[amiddavid]

Reviewer	Kind	Started by	Context	Review	Round	Head	Verdict
Reviewer 1 (glm, user of @amiddavid)	GLM rits/zai-org/glm-5-3 via Claude Code 2.1.316	offload-review skill, @amiddavid's session	resumed	1 of 2	2 of 2	d23257c90662	approve

Verdict: approve

Findings

None.

Round-1 findings, re-checked

1. Mine, negative cache_write_frac (table.go) — fixed. ParseTable now rejects cache_write_frac < 0 (table.go:97-99), with 0 keeping its "default" meaning. negfrac_test.go covers both cases. The Python reader rejects it too (kv_ttl_cost_model.py:528), with a test in test_cache_read_tiers.py. The PR body names the change and records the owner's decision. This is the fix I proposed.
2. Mine, non-blocking, LiteLLM negative stated rate — fixed. litellm.go:220-226 restores a negative stated rate after Resolved, so negative feed data prices exactly as on base. The PR body says so.
3. Mine, non-blocking, parity-test skip — fixed. The knownWriteDiff branch is gone. Python now fills a blank cache_write per family, and the test compares all 5 fields for every id.
4. Mine, non-blocking, PR-body vs docs figures — fixed. Both places now say 24x and 49x.
5. Reviewer 2's finding, 25x/50x vs 24x/49x — fixed, and their math is right. I checked it myself: the 11.5 anchor is the net formula (1.25 − 0.1) / 0.1, so the consistent pair is (1.25 − 0.05) / 0.05 = 24 and (1.25 − 0.025) / 0.025 = 49. app.js and cache-keepalive.md now carry 24x and 49x.
6. Reviewer 2's non-blocking, 7x copied fixture block — fixed. The model list and check moved to internal/modelinfo/ratefilltest; each package keeps only its own test. I checked that no non-test file imports it.

Non-blocking

• deploy/harbor/kv_ttl_cost_model.py:363 — charges_cache_write_premium strips a whole-id bracket group where Go does not: the id [claude] reads non-Anthropic in Python (the regex removes the whole id) and Anthropic in Go (stripVariant keeps a group at position 0). Pathological id, offline research tool, not covered by the parity list. Fine to leave.

What I checked

• Last review: round 1, 05907c8, 2026-11-02T16:17:02Z.
• New commits read: 1 — d23257c "refactor(modelinfo): one function decides a cache price for a model (#856)" (the PR was squashed; git diff 05907c87..HEAD covers the delta, 20 files).
• Read in full: 2 reviews, 5 comments, 0 inline comments (from .review-in-reviews.txt, .review-in-comments.txt, .review-in-inline.txt).
• New items since my last review: the squash commit d23257c; 5 author comments at 16:54 (one per finding); the updated PR body. I checked each reply against the diff, and the PR body against the code.
• The delta, read around each change:
  • table.go negative-frac validation: ordering is right (reject < 0 before the == 0 default swap); negfrac_test.go pins both cases.
  • litellm.go negative-rate restore: equal to base, where a fill fired only on == 0, so a negative stated rate survived.
  • kv_ttl_cost_model.py: hand-traced _gpt_56_or_later and charges_cache_write_premium against Go's GPT56OrLater and IsClaude case by case (no digits, major ≠ 5, . then no digits, minor ≥ 6, uppercase ids, [1m] suffixes, rfind vs LastIndex). They agree on every real id. The write fill uses the entry's own match, as Go does. The or-based default maps 0 and −0.0 to the default, which matches Go's == 0 check.
  • Parity test without the skip: recomputed the previously divergent ids by hand (gemini-3-flash-preview and premium at 1.0x under writefrac: 1.5, azure/gpt-5.5 at 1.0x, claude-opus-5-5 at 1.5x) — Python and Go now agree.
  • ratefilltest extraction: mechanical; no leftover ratefillModels / ratefillCheck / ratefillF references; fixtures unchanged from round 1, so the parity pins still hold.
  • TestAgentClassRatesMatchPrice (both packages): the 1e-12 tolerance is sound. Go constant arithmetic is exact, so the literal is 0.3 while the run-time product is 0.30000000000000004 — a difference of about 4e-17. claude-sonnet-4-5 reads 0.1x and writes 1.25x, matching both literals.
  • app.js and cache-keepalive.md: net-formula figures 11.5x / 24x / 49x, consistent with the PR body.
• Live tests were not run (not approved). All verification is by reading and hand computation.

[amiddavid]

Reviewer	Kind	Started by	Context	Review	Round	Head	Verdict
Reviewer 2 (glm, user of @amiddavid)	GLM rits/zai-org/glm-5-3 via Claude Code 2.1.316	offload-review skill, @amiddavid's session	resumed	2 of 2	2 of 2	d23257c90662	approve

Verdict: approve

Findings

None. Every round-1 finding is fixed, and the new commit introduces no defect I can show.

Earlier findings

1. Mine, round 1 — the 25x/50x ping ratios. Fixed. dash/ui/app.js:6538 and docs/how-to/cache-keepalive.md:33 now say 24x and 49x. Both use the page's own net formula (1.25 − r) / r, which gives 11.5, 24 and 49. The PR body states the same three numbers.
2. Reviewer 1, round 1 — a negative cache_write_frac changed behavior while the PR said "no behavior change". Fixed, and I checked the fix myself. ParseTable (table.go:97) now rejects cache_write_frac < 0; 0 keeps its "use the default" meaning. negfrac_test.go covers both cases. The Python reader rejects it too (kv_ttl_cost_model.py:528), with a test in test_cache_read_tiers.py. The PR body now names this change at the top.
3. Reviewer 1, non-blocking — a negative stated cache rate in the LiteLLM feed. Fixed. litellm.go:220 restores a negative stated rate after Resolved(k), so the feed prices invalid data exactly as main did (fill only when the tier is 0). I traced old vs new: the restore is unconditional and uses the raw feed value, same as base.
4. Mine and reviewer 1, non-blocking — the fixture block copied into 7 packages. Fixed. The model list and the check now live in internal/modelinfo/ratefilltest; each package keeps only its test and fixture. No non-test file imports it.
5. Mine, non-blocking — the parity test skipped Python's write tier for non-premium families. Fixed by making the two sides agree instead. See below.

What I checked in the new commit

• The Python port's new family rule, line by line against Go:
  • charges_cache_write_premium matches IsClaude ("claude" or "anthropic" substring, lowercased, [...] variant stripped) OR GPT56OrLater.
  • _gpt_56_or_later matches Go's GPT56OrLater: last "gpt-" token, major ≠ 5 → major > 5; major 5 needs ".minor" with minor ≥ 6; no digits → false; "gpt-5." (dot, no digit) → false on both.
  • The blank write fill is cache_write_frac_for(match, file_frac): the file multiple for a premium match, 1.0 otherwise. That is exactly Go's WriteFrac = f.CacheWriteFrac; if !chargesCacheWritePremium(match) { 1.0 }, and Go's lookup always uses that frac now that negatives are rejected. The parity test compares all 5 fields for every id; I walked each id in its list (gemini, premium, azure/gpt-5.5, gpt-5.6-luna, the Claude ids, [1m] suffixes, the empty model) and both sides agree.
• The negative-frac rejection does not change any accepted file: base accepted cache_write_frac < 0 only by producing a negative rate, which the PR body now names as an owner decision.
• The new pinning tests (ratefill_agent_test.go in offload and proxy): the 1e-12 tolerance correctly covers the gap between the constant product (exact 0.3, untyped constant arithmetic) and the run-time product (0.30000000000000004, 4e-17 off). The proxy test pins only the read literal because proxy has no write literal; offload pins both.
• ratefilltest.Check keeps the relative testdata/ratefill_before.json path, which resolves per package under go test — same behavior as the per-package copies it replaces.
• The parity test still compiles after removing knownWriteDiff: math and modelinfo are both still used.
• Re-read the updated PR body: it names both edge-case changes (negative multiple rejected, Python tool write fill) at the top, with the owner's decision, as the repo rule requires.

Non-blocking

• internal/modelinfo/ratefilltest is a normal package that imports testing. Precedent exists (dispatch/dispatchtest); nothing outside tests imports it.
• The LiteLLM negative-rate restore has no direct test; the fixtures use non-negative costs only. The code is three lines and restores base behavior; a fixture row with a negative cost would pin it.
• Pre-existing, unchanged: the parity test's doc set has no entry with in: 0, where Go's CacheWrite1h() (input × 2) and Python's _derive_write_1h (write_5m-scaled) would take different branches. Test-coverage note only.

What I checked

• Last review: round 1, 05907c8, 2026-11-02T16:14:41Z
• New commits read: 1 (d23257c9 "refactor(modelinfo): one function decides a cache price for a model (#856)")
• Read in full: 2 reviews, 5 comments, 0 inline comments (from the three files)
• New items since my last review, and what I did about each:
  • Commit d23257c9: read the full diff (39 KB) and the code around each change; verified every fix listed above.
  • 5 comments by the author (fix notes for the 5 round-1 items): checked each claim against the code; all true.
  • Updated PR body: read in full; the behavior-change naming and owner decisions are present.
• Live tests were not run (not approved); all verification is by reading.