Correct two wrong comments, and widen gateway/Gemini attribution - #19
Conversation
Two comments in the Cloudflare gateway adapter were wrong, and they were the stated justification for code, so they would have misled the next reader. The module docstring claimed usage_metadata's key casing "is NOT normalized by Cloudflare — it passes through whatever convention the provider used". It does not. Across all 14 captured fixtures the only keys that ever appear are Cloudflare's own: input_tokens, output_tokens, total_tokens, input_cached_tokens, input_cache_creation_tokens, neurons, input_text_tokens, reasoningTokens. Not one provider-native key shows up. The cited proof — camelCase reasoningTokens — is Cloudflare's own inconsistency, not a leaked provider key: Gemini's native spelling is thoughtsTokenCount, absent everywhere. _first_int said a missed cache key is "an over-bill, not an omission". That holds only for a subtractive provider, where compute_cost subtracts cache_read out of input. For additive Anthropic the same miss means those tokens are never billed at all — an under-bill, the worse direction. The comment asserted one direction for a function used by both. Anthropic's cache_read_input_tokens and Gemini's thoughtsTokenCount join the spelling fallthrough, labelled in the code as unobserved insurance rather than handling for a known case, since neither has appeared in a fixture. Kept because the fallthrough is free and a missed cache key mis-bills either way. Gemini streaming attributed the requested alias instead of the resolved model when a chunk carried usage without model_version: this port read it off whichever chunk carried usage, the JS port remembered it across chunks, so the two priced the same call differently. It now persists across chunks (sync and async) and accepts both spellings. Not reachable with Gemini as it behaves today — every streaming chunk carries both fields, verified live, which is why the existing fixture could not catch it. That fixture is left faithful to reality; a separate, explicitly synthetic case pins the property instead.
ancorcruz
left a comment
ancorcruz
left a comment
There was a problem hiding this comment.
Review — verified, one comment still wrong
Both fixes check out. The Gemini stream now persists resolved_model across chunks and accepts both model_version and modelVersion; the gateway adapter accepts Anthropic's cache_read_input_tokens and Gemini's thoughtsTokenCount. New tests cover both. Locally: ruff, format, mypy clean, 533 unit tests pass (CI still hasn't run on this stack — see #17).
The fixture audit corrected my premise, and that's the most valuable thing in this PR. My comment asserted that the gateway "forwards each provider's own key casing through unnormalized", inferring that from reasoningTokens. You went and checked all 14 fixtures and found the opposite: Cloudflare rewrites into its own vocabulary, no provider-native key ever appears, and reasoningTokens is Cloudflare's own camelCase inconsistency — Gemini's native spelling for that quantity is thoughtsTokenCount, which appears nowhere. So the old docstring was wrong, my reasoning for the fix was wrong, and the extra spellings are unobserved insurance rather than handling for a known case. Labelling them that way in the code is exactly right; a future reader would otherwise inherit my mistake.
The over-bill/under-bill split you added is also a better statement than mine — I only described the additive-Anthropic direction, and it does invert by provider.
One comment in the same family is still wrong, and it's not in this diff. adapters/openai_native.py:127, in _infer_provider:
"...and what a streaming call always reports (the synthetic usage payload carries no model, so
resolve_modelfalls back to the requested string verbatim)."
b2d7382 on #13 made that false in the same change set that wrote it — _extract_stream_usage now returns {"usage": …, "model": payload.get("model")}, so a streaming call reports whatever the gateway echoes, not the requested string. Still present at the tip of this stack.
The docstring is the small half. The open question underneath it is whether Cloudflare's /compat streaming chunks echo model as workers-ai/@cf/…, bare @cf/…, or something else entirely — because _infer_provider only recognises the first two. If it echoes a bare slug, a streamed Workers AI call is stamped openai, looked up against OpenRouter, and degrades to token events. One captured streaming chunk settles it, and it's worth a fixture either way: it's the single path where the two halves of the Workers AI fix can disagree, and it's the last item from the original review that's neither fixed nor explicitly deferred.
Still deliberately deferred and fine by me: prime()/TTL, and the 40-page catalog walk on the queue thread.
| # differently. Both spellings are accepted because | ||
| # `model_dump()` yields snake_case while a raw REST dict | ||
| # is camelCase. | ||
| mv = payload.get("model_version") or payload.get("modelVersion") |
There was a problem hiding this comment.
Minor: this block and its 14-line comment are now byte-identical to the sync path at line 113.
Pre-existing shape rather than something this PR introduced — but the duplicated region just grew from 4 lines to ~20, comment included, and the two copies have to stay in agreement for a reason the comment itself spells out (the ports priced the same call differently when one side lagged).
A module-level helper taking the payload dict and the current resolved_model would collapse both:
def _resolved_version(payload: dict, current: str | None) -> str | None:
mv = payload.get("model_version") or payload.get("modelVersion")
return mv if isinstance(mv, str) and mv else currentThen each loop is resolved_model = _resolved_version(payload, resolved_model), the comment lives in one place, and wrappers/anthropic.py's _merge_stream_usage has a sibling with the same shape. Not worth blocking on.
|
Answered the open question empirically, since it was the last item from the original review that was neither fixed nor deferred. Captured a real streaming The feared outcome does not happen, though. Follow-up branch will carry the corrected docstring plus that captured chunk as a fixture, so it is pinned rather than re-derived. Full detail on the On the duplicated block in |
2 participants
Answers two threads, and corrects two comments of mine that were wrong.
cache_read_input_tokensand Gemini'sthoughtsTokenCountadded. But I checked the fixtures before writing code, and the justification I'd given for the existing entries was false: across all 14 captured fixtures the only keys that ever appear are Cloudflare's own. Not one provider-native key. The camelCasereasoningTokensI'd cited as proof is Cloudflare's own inconsistency — Gemini's native spelling isthoughtsTokenCount, which appears nowhere. So the new entries are labelled in the code as unobserved insurance, not handling for a known case. The stale module docstring is rewritten to match.model_versiononly from the usage chunk — confirmed, and it was a real divergence: this port read it off the usage-bearing chunk, JS remembered it across chunks, so the two priced the same call differently. Now persisted (sync and async). Not reachable with Gemini as it behaves today — every streaming chunk carries both fields, verified live, which is exactly why the existing fixture couldn't catch it. I left that fixture faithful to reality rather than editing it to expose the bug, and added an explicitly synthetic case beside it.Second correction:
_first_int's comment said a missed cache key is "an over-bill, not an omission". That's true only for a subtractive provider. For additive Anthropic the same miss means those tokens are never billed at all — an under-bill, the worse direction. It asserted one direction for a function used by both.Green at this commit: ruff +
ruff format --check+ mypy clean, 533 unit tests.