fix: provider usage accounting, retry/overflow classification, and output clamping#109796
Conversation
openclaw-barnacle
Bot
added
docs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d814961c61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const maxTokens = clampMaxTokensToContext(model, context, adjusted.maxTokens); | ||
| const thinkingBudget = clampThinkingBudgetToMaxTokens(maxTokens, adjusted.thinkingBudget); |
There was a problem hiding this comment.
When a budget-based Anthropic model is near the context limit, clampMaxTokensToContext can reduce max_tokens below the previously adjusted thinking budget plus the helper's 1024-token answer reserve; this line then clamps the thinking budget to max_tokens - 1 (for example 1500/1499), leaving only one token for the actual reply. The Anthropic SDK contract for thinking.budget_tokens says it counts toward max_tokens, so these requests can spend the entire output budget on thinking and return a max_tokens stop instead of a usable answer; reapply the existing min-output-budget logic after the context clamp or disable/reduce thinking in this case.
Useful? React with 👍 / 👎.
d814961 to
8e955eb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e955ebf59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /** Whether empty thinking signatures can be replayed as native thinking blocks. Default: false. */ | ||
| allowEmptySignature?: boolean; |
There was a problem hiding this comment.
When this new compat knob is needed by a configured or catalog Anthropic-compatible model, it cannot actually be set: the config type allowlist only picks supportsEagerToolInputStreaming/supportsLongCacheRetention, the runtime TypeBox compat schema omits it, and the model-catalog normalizer drops unknown boolean fields. As a result normal model loading rejects or strips compat.allowEmptySignature, so getAnthropicCompat() always falls back to false outside tests that construct Model objects directly, and empty-signature replay still gets converted to text for those providers.
Useful? React with 👍 / 👎.
8e955eb to
43d701f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43d701faa8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| textTokens += | ||
| textTokenUpperBound(tool.name) + | ||
| textTokenUpperBound(tool.description) + | ||
| serializedTokenUpperBound(tool.parameters); |
There was a problem hiding this comment.
When streamSimpleAnthropic/streamSimpleMistral pass the context into buildBaseOptions, this estimator now runs before the provider's existing tool projection/quarantine. If a direct/custom tool has an unreadable parameters getter (a case the provider tests already preserve by skipping the bad tool and keeping healthy siblings), this property read throws synchronously, so simple streams fail before streamAnthropic/streamMistral can apply that safe fallback. Snapshot descriptor fields under the same guarded path, or reuse the provider projection rather than reading raw tool descriptors here.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| function textTokenUpperBound(value: string): number { | ||
| return UTF8_ENCODER.encode(value).byteLength; |
There was a problem hiding this comment.
This byte length is subtracted from model.contextWindow, which is measured in tokens. For large ASCII/code prompts the byte count can be several times the real tokenizer count, so a prompt like ~130k ASCII characters on a 128k-token model can still be well under the model window but gets clamped to the 1-token floor, yielding effectively empty replies instead of preventing only true provider validation overflows. Use a less destructive estimate/fallback, or reserve hard clamping for actual token counts.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed July 17, 2026, 7:21 AM ET / 11:21 UTC. Summary PR surface: Source +166, Tests +610, Generated 0. Total +776 across 18 files. Reproducibility: yes. at source level: deterministic fixtures exercise omitted Anthropic usage, retry and overflow strings, and cache accounting. The claimed real-provider runs are not yet inspectable enough to verify the complete current-head runtime path. Review metrics: 2 noteworthy metrics.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Rebase onto current main, keep retry policy at the app-owned outer-retry boundary unless maintainers explicitly approve a public default change, replace byte-based hard clamping with a token-aware or non-destructive guard, and propagate replay and usage invariants through model loading and every Anthropic transport before attaching redacted live output. Do we have a high-confidence way to reproduce the issue? Yes at source level: deterministic fixtures exercise omitted Anthropic usage, retry and overflow strings, and cache accounting. The claimed real-provider runs are not yet inspectable enough to verify the complete current-head runtime path. Is this the best way to solve the issue? No. The classifier and accounting changes are plausible, but the patch is currently too one-sided across public API, model loading, boundary-aware transport, and output-budget estimation to be the best maintainable fix. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ecfcaa07e6d4. Label changesLabel justifications:
Evidence reviewedPR surface: Source +166, Tests +610, Generated 0. Total +776 across 18 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (4 earlier review cycles)
|
274425b to
8723179
Compare
clawsweeper
Bot
added
rating: 🦪 silver shellfish
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06d49522d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
| } else { | ||
| output.usage.contextUsage = { state: "unavailable" }; | ||
| if (event.usage) { |
There was a problem hiding this comment.
When an Anthropic model runs through the boundary-aware transport (src/agents/provider-transport-stream.ts selects src/agents/anthropic-transport-stream.ts for embedded/proxied/transported anthropic-messages), that sibling stream still handles message_delta without this event.usage guard: an omitted usage object falls through to contextUsage = { state: "unavailable" }, discarding the valid message_start prompt snapshot. The package provider test added here passes, but production agent runs on that transport path can still lose context accounting and overflow-compaction signals; mirror the guard there too.
Useful? React with 👍 / 👎.
| const cacheWriteUsage = readAnthropicCacheWriteUsage(event.message.usage); | ||
| if (cacheWriteUsage.cacheWrite1h !== undefined) { | ||
| output.usage.cacheWrite1h = cacheWriteUsage.cacheWrite1h; |
There was a problem hiding this comment.
With long Anthropic prompt caching on the boundary-aware transport path, the sibling stream still only copies cache_creation_input_tokens and never calls readAnthropicCacheWriteUsage, so usage.cacheWrite1h stays undefined. Because the new cost calculation relies on that field to price 1-hour writes at input * 2, embedded/proxied Anthropic runs continue to bill all cache writes at the 5-minute rate while the package provider reports a different total.
Useful? React with 👍 / 👎.
06d4952 to
5b917ff
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b917ff5a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /** Subset of `cacheWrite` written with 1-hour retention when reported. */ | ||
| cacheWrite1h?: number; |
There was a problem hiding this comment.
When an Anthropic response with 1-hour cache writes is replayed or returned over worker inference, this new Usage field is not accepted by the closed worker usage schemas in packages/gateway-protocol/src/schema/worker-inference.ts and worker-admission.ts, and the terminal projection in src/gateway/worker-environments/inference-terminal-message.ts also drops it. That means a transcript containing usage.cacheWrite1h can be rejected as an invalid worker context/terminal outcome, or the new billing bucket disappears on the worker path even though the package provider emits it.
Useful? React with 👍 / 👎.
5b917ff to
60405ef
Compare
clawsweeper
Bot
added
rating: 🧂 unranked krab
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60405efce9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const rawCachedTokens = | ||
| record.promptTokensDetails?.cachedTokens ?? | ||
| record.prompt_tokens_details?.cached_tokens ?? | ||
| record.cachedTokens ?? | ||
| record.cached_tokens; |
There was a problem hiding this comment.
When a real Mistral stream returns cache usage, the SDK has already parsed usage into UsageInfo; Mistral documents cached tokens at usage.prompt_tokens_details.cached_tokens, but the current TS SDK keeps unmodeled wire fields under additionalProperties in UsageInfo. These reads only check top-level camel/snake fields, so real cache hits still report cacheRead: 0 and bill all prompt tokens as uncached; include record.additionalProperties?.prompt_tokens_details?.cached_tokens or otherwise preserve that SDK extra field.
Useful? React with 👍 / 👎.
# Conflicts: # docs/.generated/plugin-sdk-api-baseline.sha256
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfe3a8041f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cacheRead: number; | ||
| cacheWrite: number; | ||
| /** Subset of `cacheWrite` written with 1-hour retention when reported. */ | ||
| cacheWrite1h?: number; |
There was a problem hiding this comment.
When Anthropic long-retention responses reach normal agent/session accounting, this new field is dropped by normalizeUsage (src/agents/usage.ts) and the run/session cost estimator still prices cacheWrite only as cacheWrite * cost.cacheWrite (src/utils/usage-format.ts). That means embedded/gateway usage summaries and session cost totals continue billing 1-hour cache writes at the 5-minute cache-write rate even though the provider stream now records the correct bucket; carry cacheWrite1h through normalized usage/accumulation and apply the same 2x-input pricing there.
Useful? React with 👍 / 👎.
# Conflicts: # docs/.generated/plugin-sdk-api-baseline.sha256
|
Merged via squash.
|
…tput clamping (openclaw#109796) * fix(ai): provider usage accounting, retry and overflow classification, output clamping * ci: refresh pull-request validation * test(ai): live-probe overflow classification and output clamping
1 participant
What Problem This Solves
Provider-boundary correctness for Anthropic and Mistral plus shared retry/overflow hardening:
usagefrommessage_deltaevents.cacheWritefield intact.allowEmptySignaturecompat flag preserves them (default unchanged).usage.cacheRead.ResourceExhausted— all transient, all previously fatal.Why This Change Was Made
These are correctness gaps users hit as crashed streams, silently wrong cost numbers, stuck sessions after context overflow, and hung retries. The classifiers stay purely classificatory; policy remains with the caller.
User Impact
Anthropic streams tolerate lean proxies; cost reporting is accurate under 1h cache retention; Mistral costs drop via real prompt caching; transient CF/socket/gRPC failures retry instead of failing hard; overflow recovery triggers reliably; near-full-context requests degrade gracefully instead of erroring.
Evidence
node scripts/test-live.mjs -- packages/ai/src/providers/anthropic.live.test.ts packages/ai/src/providers/mistral.live.test.ts— 4/4 passing: Anthropic stream with usage, 1h cache-retention request parsingcache_creationbuckets with correct cost accounting, thinking-signature survival, and a Mistral stream with cache-field parsing.Usage.cacheWrite1h/allowEmptySignaturecontracts;pnpm plugin-sdk:api:checkgreen post-rebase.Additional live probes (real API keys): overflow classification proven against providers' current error strings — a deliberately context-exceeding request to Anthropic and to OpenAI Responses each classify as overflow via
isContextOverflow(guarding compaction recovery against provider error-string drift), and the new output-clamp path completes successfully on a near-full context instead of a validation 400. Anthropic+Mistral 6/6, OpenAI Responses 4/4 keyed runs.