ci: verify #818 head (do not merge) - #864
Merged
Merged
Conversation
`token_safety_margin` gains a rung between the tiktoken-is-exact check and `DEFAULT_TOKEN_SAFETY_MARGIN`. When a provider has counted the request, the margin becomes the measured ratio of its count to our local one, rather than the 1.5 that covers the worst content anyone measured. Deliberately a ratio and not a total. The margin is applied per component, `BudgetPlan.total_input_tokens` is a computed property summing those parts, and the planner recounts on mutated text inside its trim loop. A recorded total stops being true the moment anything is trimmed; a ratio stays approximately valid. Clamped at 1.0: a provider counting below tiktoken may only reduce over-inflation, never reclaim window. Rejected outright on a model-name mismatch, a zero baseline, a non-positive count, or a non-finite ratio, each of which falls through to the existing ladder — so an absent or broken measurement is the 1.5 guess, by construction rather than by a second code path remembering. The "estimated, not exact" warning stays quiet only when the measurement actually supplied the margin. Inert until a caller passes one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HucxUJ7gaA6iKveF8aruW2
`_sized_for` re-derived the current model's safety margin from its name and config in order to restate a request in a candidate's units. That is correct only while every count comes from the same estimate-plus-default path. A provider-counted request carries a much tighter margin, and `token_safety_margin` cannot know that happened — it still answers 1.5. Dividing an already-tight number by a factor never applied understates the request by the whole difference, at the exact point the router is deciding whether a candidate can hold it. That is the #648 defect — an estimate that reads low, so the request hard-fails — relocated to the routing boundary, which this function's own docstring says it exists to prevent. Callers may now state the margin their number was measured with. `choose_document_model` and `suggest_document_model` pass it through; the candidate's margin stays derived, because the candidate has not been counted. Omitted, every caller keeps today's arithmetic to the token. A supplied margin below 1.0, non-finite, or not a number at all is refused in favour of the honest derivation, matching how `context_budget._configured_margin` refuses the same thing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HucxUJ7gaA6iKveF8aruW2
New `native_token_count`: fetches a provider's own count for a prepared request, or reports plainly that it could not. Two properties, and the second is the load-bearing one. A returned figure is the provider's own, unmodified. Anything going wrong returns nothing at all — the caller drops its safety margin on the strength of a usable result, so a count that comes back zero, short, or from a provider that counted half the request is worse than no count, converting a conservative over-estimate into a confident under-estimate. So: gated before any I/O on a protocol that can actually count, since `OpenAIModel.count_tokens` raises and every OpenAI-compatible model would otherwise decrypt a key and build a client per chat turn to learn that. Wrapped in a five-second timeout that is deliberately not the request's own settings — `build_thinking_model_settings` sets 120s and the shared client retries a 429 six times honouring Retry-After up to 60s, which would put a two-minute stall in front of every message. Blanket `except` by necessity: Anthropic raises ModelHTTPError, Google's genai errors are unwrapped, and a misconfigured protocol reaches OpenAIModel; there is no complete and stable union of those. CancelledError passes through, being the caller going away rather than a failure. Google's count omits the system instruction — pydantic-ai attaches `system_instruction` only for providers other than `google-gla`, and an api-key GoogleProvider is `google-gla`. `covers_system_prompt` says so rather than letting a caller silently drop a multi-kilobyte grounding preamble from its budget. Nothing imports this yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HucxUJ7gaA6iKveF8aruW2
Wires the native count into `chat_stream`. Before the request is sized, the provider is asked what the assembled prompt actually costs; the same payload is counted locally; the ratio becomes this turn's safety margin in place of the 1.5 guess. The payload counted is the whole user prompt, documents included. `_build_chat_prompt` is the existing assembly extracted verbatim so the pre-flight counts the request chat is about to make rather than a reconstruction of it — in document chat the documents are the payload, and they are also the digit-dense content the 1.5 was sized for. Sampling the question and history alone would measure prose and apply the answer to a budget table. The baseline covers exactly the components the provider counted, which differ by provider: Anthropic's count includes the system prompt, Google's does not for api-key providers. Counting it on one side and not the other would depress the ratio for a reason unrelated to tokenizer divergence, and a depressed ratio clamps to a margin of 1.0 over a tiktoken figure that itself under-counts these models — the hard-fail direction. Routing is given the margin the number was measured with, rather than re-deriving it. That value survives a model switch on purpose: it describes how `requested_input_tokens` was measured, and that does not change when the model does. A count for one model is not passed to the planner for another. `_native_margin` already rejects on a name mismatch, but relying on that silently is how the next person learns it the hard way. Anything unusable — ineligible protocol, timeout, 429, malformed response — leaves the stream exactly as it is today. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HucxUJ7gaA6iKveF8aruW2
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HucxUJ7gaA6iKveF8aruW2
The module coalesced the failure log but not the failure work. A deployment whose egress to the count endpoint is slow, or whose org is rate-limited there, paid the full 5s timeout on every chat turn ahead of the first token -- permanently, and after the first occurrence it said so only at DEBUG. There was no metric and no way out short of a restart. Three consecutive failures now pause a model for five minutes. The check sits after the protocol gate, so an ineligible model never enters the circuit, and before any model construction, key decryption or request, so an open circuit costs nothing. One request is let through when the cooldown elapses, and any success clears the record, so an outage cannot disable counting until the next deploy. State is per model name: a failing Anthropic endpoint does not stop Gemini being counted. The clock is monotonic, so a clock adjustment cannot strand a model in cooldown. Skipping is strictly better than blocking here: the estimate path it falls back to is the one that shipped before this feature, so a paused model gets the same budget it would have got anyway, without the wait. Two corrections to the changelog, both things it asserted that the code does not do: - "fires only for models an admin explicitly configured" is not true of Gemini. detect_api_protocol routes any name containing "gemini" to google on its own, so a deployment already running one gets the pre-flight on upgrade without changing anything. Issue #816 said this correctly and the entry lost it. - "one unconditional round trip" is no longer true, which is the point of this commit. The existing failure-logging tests needed updating rather than working around: _CIRCUIT is per-process state exactly like _UNAVAILABLE_LOGGED, so it joins that autouse reset -- without it, whether a test sees an open circuit depends on collection order. The repeated-failure test now runs 50 turns rather than 4, pinning the bound instead of an arithmetic coincidence. Verified by disabling only the gate while keeping the state: the provider is called on all 10 turns and the test fails on the count. Stashing the whole module instead only proves a symbol is missing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CtRYNNEdrq6SPyPt8PjH1v
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CI-only. The maintainer commits pushed to #818's fork branch did not trigger a workflow run — its green checks are for the contributor's original commit, not for the review fixes. This mirrors the exact tree so the fixes are actually verified before #818 is merged. Close once #818 lands.