Repository navigation
📊 feat: Emit ON_CONTEXT_USAGE Token Budget Snapshots - #235
Conversation
|
@codex review |
98bf2ca to
2530383
Compare
![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: 98bf2ca86c
ℹ️ 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".
| contextBudget, | ||
| effectiveInstructionTokens, | ||
| prePruneContextTokens, | ||
| remainingContextTokens, |
There was a problem hiding this comment.
When calibration has moved away from 1 and the pruner actually drops messages, remainingContextTokens is in raw token-counter units: createPruneMessages calls getMessagesWithinTokenLimit with rawSpaceBudget/rawSpaceInstructionTokens and forwards that raw remaining value back. Emitting it next to provider-space contextBudget makes consumers compute contextBudget - remainingContextTokens incorrectly for the context gauge (e.g. a 2x calibration reports too much occupancy after pruning). Scale the remaining value back to calibrated/provider units before including it in the event, or return a calibrated remaining value from the pruner.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bc73f83 — the pruned path now scales the raw-space remaining back by the calibration ratio (clamped to the budget) before returning, so contextBudget - remainingContextTokens is consistent provider-space math on both pruner paths. Added a regression test asserting the calibrated value when pruning occurs with ratio 2.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f18e6b6d46
ℹ️ 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".
| } | ||
| messagesToUse = context; | ||
|
|
||
| void safeDispatchCustomEvent( |
There was a problem hiding this comment.
When summarization is enabled and pruning produces messagesToRefine, this dispatch runs before the hasPrunedMessages branch below can return a summarizationRequest; in that path no LLM call is made from this node, and the post-summary retry will emit another usage snapshot. Hosts consuming ON_CONTEXT_USAGE as the advertised per-model-call signal will therefore see a ghost/pre-compaction snapshot whenever pruning triggers summarization, which can double-count calls or briefly display a context gauge for a prompt that was never sent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 562360a — the snapshot is now built in the pruner block but dispatched adjacent to the model invoke, after the summarization-trigger decision. A summarization detour returns from the node without emitting; the post-summary retry produces the only snapshot.
| messageCount, | ||
| messageTokens, | ||
| availableForMessages, | ||
| toolTokenCounts: { ...this.toolTokenCounts }, |
There was a problem hiding this comment.
When an agent is constructed with a cached toolSchemaTokens value, fromConfig() skips calculateInstructionTokens(), leaving this.toolTokenCounts at its default {} even though toolSchemaTokens is nonzero. Returning an empty object here makes ON_CONTEXT_USAGE consumers treat per-tool schema costs as known and zero for cached runs, so any UI/debugging based on toolTokenCounts silently omits all tools; omit the field when counts were not calculated, or populate it alongside the cached aggregate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 562360a — toolTokenCounts is now undefined when not calculated (cached aggregate path) instead of an empty object, and the breakdown omits the field accordingly; consumers fall back to the aggregate toolSchemaTokens.
| { | ||
| runId: this.runId, | ||
| agentId, | ||
| breakdown: agentContext.getTokenBudgetBreakdown(messages), |
There was a problem hiding this comment.
For thinking-enabled runs that include historical AI/tool messages without a thinking block, the code below can still replace that sequence with a [Previous agent context] HumanMessage in ensureThinkingBlockInMessages after this snapshot is built. In that scenario the event reports breakdown and remainingContextTokens for the pre-format messages/context, not for the payload actually sent to the model, so hosts can show a materially wrong context gauge for migrated or handoff histories that require this normalization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 562360a — dispatch moved after all post-prune formatting (cache control, thinking normalization, orphan sanitize). When formatting restructures the payload (length change, e.g. the [Previous agent context] collapse), the message tokens are recounted and remainingContextTokens recomputed against the calibrated budget before emitting.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
https://github.com/danny-avila/agents/blob/562360ab4c0d5105af819153af0314976f5e0f06/package-lock.json#L5635
This lockfile update strips the libc constraints from the Linux @rolldown native bindings, while npm documents libc as the Linux-specific selector alongside os/cpu. On Linux x64/arm64 installs, npm can now consider both the glibc and musl variants installable from the lockfile, which can install the wrong native binary or both and break builds in Alpine or glibc CI. Please regenerate the lockfile with a toolchain that preserves these package libc fields, or restore the removed selectors for the affected @rolldown/binding-linux-* entries.
ℹ️ 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".
| if (contextUsage != null) { | ||
| if ( | ||
| agentContext.tokenCounter != null && | ||
| finalMessages.length !== messagesToUse.length |
There was a problem hiding this comment.
When a post-prune formatter changes the messages without changing the array length, this guard skips the recount and leaves remainingContextTokens based on the pre-formatted context. For example, useLegacyContent calls formatContentStrings(finalMessages), which returns a new array of the same length and mutates structured text blocks into strings before the model call, but the context-usage event still reports the old token total. Use the existing finalMessages !== messagesToUse signal, or recount whenever formatting may have reassigned/mutated the payload, so the snapshot reflects what is actually sent.
Useful? React with 👍 / 👎.
| contextUsage = { | ||
| runId: this.runId, | ||
| agentId, | ||
| breakdown: agentContext.getTokenBudgetBreakdown(messages), |
There was a problem hiding this comment.
When provider calibration changes toolSchemaTokens in the block above, this snapshot is built after that mutation while remainingContextTokens and effectiveInstructionTokens still come from the pruner calculation that used the previous instruction budget. On a calibrated turn where tool overhead variance crosses the threshold, the emitted breakdown.instructionTokens/availableForMessages no longer reconciles with remainingContextTokens, so consumers can show impossible context-usage totals. Capture the breakdown before mutating toolSchemaTokens, or recompute the event values from the same calibrated instruction total.
Useful? React with 👍 / 👎.
| const deferredToolNames: string[] = []; | ||
| for (const [name, rawCount] of Object.entries(rawToolTokenCounts)) { | ||
| toolTokenCounts[name] = Math.ceil(rawCount * toolTokenMultiplier); | ||
| if (this.toolRegistry?.get(name)?.defer_loading === true) { |
There was a problem hiding this comment.
For event-driven agents that provide deferred toolDefinitions without a separate toolRegistry, discovered deferred definitions are counted by getActiveToolDefinitions(), but this check only looks in toolRegistry, so deferredToolNames stays empty even though a counted tool has defer_loading: true. Consumers of the context-usage event then cannot identify which discovered deferred tools are contributing tokens. Use the active definition's own defer_loading flag, or otherwise populate this list from both sources.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5553952 — deferred status is now captured from the active definition's own defer_loading flag during counting (with the registry lookup kept as fallback), so event-driven agents without a registry entry still report deferredToolNames.
| void safeDispatchCustomEvent( | ||
| GraphEvents.ON_CONTEXT_USAGE, | ||
| contextUsage, | ||
| config | ||
| ); |
There was a problem hiding this comment.
Because this dispatch runs before the empty-prompt guard below, a run where pruning leaves finalMessages.length === 0 and summarization does not take over still emits ON_CONTEXT_USAGE and then throws empty_messages before invoking the model. Hosts listening for the new per-model-call event can update usage UI for a call that never happened. Move the dispatch after the empty-message validation, or suppress it on this error path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5553952 — the dispatch (with its recount) now sits after the empty-messages guard, so a doomed prompt throws before any snapshot is emitted; the event fires only when a model call is guaranteed.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 555395214f
ℹ️ 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".
| agentContext.tokenCounter != null && | ||
| finalMessages.length !== messagesToUse.length |
There was a problem hiding this comment.
When post-prune formatting mutates message content without changing the array length, this guard skips the recount and emits an ON_CONTEXT_USAGE snapshot based on the pre-format prompt. For example, the Anthropic path calls formatAnthropicArtifactContent(finalMessages), which can append artifact blocks into an existing ToolMessage without adding/removing messages, and formatContentStrings can also rewrite content in place. In those contexts the UI receives stale remainingContextTokens even though the actual prompt sent to the model is larger or differently encoded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining this one with reasoning: the snapshot is an estimate by design, and unconditional recounting would retokenize every message on every model call — exactly the per-call cost the memoized indexTokenCountMap accounting exists to avoid. In-place content rewrites (artifact appends, legacy string formatting, cache-control markers) produce bounded deltas that the calibration loop absorbs: calibrationRatio is derived from provider-reported input vs. local estimates, so systematic formatting overhead self-corrects after the first call. The length-change recount targets the one case calibration cannot absorb — structural collapses that change counts by orders of magnitude (thinking-placeholder, orphan drops). If a concrete formatter is shown to produce large in-place deltas, the cheap targeted fix is recounting just the mutated tail message, not the prompt.
| toolTokenCounts: | ||
| this.toolTokenCounts != null ? { ...this.toolTokenCounts } : undefined, | ||
| deferredToolNames: | ||
| this.deferredToolNames.length > 0 | ||
| ? [...this.deferredToolNames] |
There was a problem hiding this comment.
In the deferred-tool flow, markToolsAsDiscovered only marks the system prompt stale, so the next model call can bind a newly discovered deferred tool while toolTokenCounts and deferredToolNames still reflect the old calculation. Because ON_CONTEXT_USAGE now exposes these cached fields directly, the event omits the newly loaded tool from the per-tool breakdown in that scenario, causing consumers to under-report the tool schema usage for the call that first uses the discovered tool.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fd45bd5 — markToolsAsDiscovered now re-triggers calculateInstructionTokens (awaited via tokenCalculationPromise before the next model call), so both the aggregate and the per-tool breakdown include newly discovered tools. This also fixes the pre-existing staleness the old docstring acknowledged for toolSchemaTokens.
| contextUsage = { | ||
| runId: this.runId, | ||
| agentId, | ||
| breakdown: agentContext.getTokenBudgetBreakdown(messages), |
There was a problem hiding this comment.
When a graph config sets summarizationConfig.reserveRatio, the pruner uses that value to compute contextBudget, but this emitted breakdown still calls getTokenBudgetBreakdown, which hard-codes DEFAULT_RESERVE_RATIO for availableForMessages. In those runs the same ON_CONTEXT_USAGE payload reports two different message budgets, so consumers using breakdown.availableForMessages will show an incorrect context gauge whenever the reserve ratio differs from the default.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fd45bd5 — getTokenBudgetBreakdown now mirrors the pruner's reserve math (summarizationConfig.reserveRatio ?? DEFAULT_RESERVE_RATIO, same bounds guard), so availableForMessages agrees with contextBudget in the same payload.
| toolTokenCounts: | ||
| this.toolTokenCounts != null ? { ...this.toolTokenCounts } : undefined, |
There was a problem hiding this comment.
After provider calibration, Graph can replace agentContext.toolSchemaTokens with a calibrated aggregate when the variance threshold is exceeded, but the newly exposed toolTokenCounts here remain the original local per-tool estimates. In that calibrated-provider scenario the event payload contains an aggregate toolSchemaTokens that no longer matches the per-tool breakdown, so hosts cannot accurately attribute tool overhead unless these counts are recalibrated or omitted with the aggregate override.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fd45bd5 — when calibration overrides the aggregate past the variance threshold, the per-tool counts are scaled proportionally by the same factor, so the breakdown keeps summing to toolSchemaTokens while preserving relative attribution.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd45bd5e8a
ℹ️ 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".
| if ( | ||
| agentContext.tokenCounter != null && | ||
| finalMessages.length !== messagesToUse.length |
There was a problem hiding this comment.
When post-prune formatting changes message content but preserves the array length, this guard skips the recount, so ON_CONTEXT_USAGE.remainingContextTokens stays based on the pre-format payload even though a different payload is sent to the model. For example, useLegacyContent runs formatContentStrings(finalMessages) just above and can rewrite text-block content into strings without changing finalMessages.length; Anthropic/Bedrock cache-control or artifact formatting can similarly change content in place. The usage gauge will under/over-report occupancy for those provider/configuration paths, so the recount should not depend only on a length change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Implemented in 24e91aa — you're right that the in-place formatters (artifact appends, Bedrock content rewrite, legacy string conversion) are invisible to both length and identity checks, so I went with a bounded compromise rather than full retokenization: the trailing two messages (the only targets of in-place mutation today) are counted before the formatting chain and again at dispatch, and remainingContextTokens is adjusted by the calibrated tail delta (clamped to the budget). Constant cost per call — two extra tokenCounter invocations — while the length-change path still triggers the full recount for structural rewrites. Head-of-array conversions in legacy mode remain uncounted by design: they're text-preserving representation changes with ~zero delta, and the calibration loop absorbs residue. Also updated the AgentContext test that pinned the old discovery-staleness behavior (the CI failure) to assert the refresh instead.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24e91aa7e7
ℹ️ 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".
| contextUsage = { | ||
| runId: this.runId, | ||
| agentId, | ||
| breakdown: agentContext.getTokenBudgetBreakdown(messages), |
There was a problem hiding this comment.
When pruning actually drops earlier turns, this passes the original messages array into getTokenBudgetBreakdown, so breakdown.messageCount and breakdown.messageTokens describe the pre-prune conversation while remainingContextTokens describes the post-prune prompt that will be sent. Consumers of the new ON_CONTEXT_USAGE event cannot reconcile the budget snapshot in pruned conversations; use the pruned context/messagesToUse for the breakdown instead, leaving prePruneContextTokens as the pre-prune metric.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d271673 — the event's breakdown now describes the post-prune prompt: messageCount from the kept context, messageTokens derived from the same calibrated budget math as remainingContextTokens (summing the pre-prune-keyed index map over the pruned array would missum, which is why the original array was used). prePruneContextTokens remains the pre-prune metric.
| if (contextUsage != null && agentContext.tokenCounter != null) { | ||
| preFormatTailTokens = 0; | ||
| for (const message of messagesToUse.slice(-2)) { | ||
| preFormatTailTokens += agentContext.tokenCounter(message); |
There was a problem hiding this comment.
For Anthropic-like multi-tool turns, formatAnthropicArtifactContent mutates every ToolMessage after the latest AI tool-call, but this snapshot only covers the last two messages. If an AI call returns three or more tool results with artifacts, the dispatch-time adjustment misses the earlier mutated tool result(s) and reports remainingContextTokens too high for the actual prompt. Recount all formatted messages, or at least all tool messages in the trailing tool batch, when this formatter can run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d271673 — the tail window is now the trailing tool batch plus its owning AI message (capped at 16) via trailingMutationStart, covering multi-tool artifact mutations and the Bedrock rewrite; same boundary used for pre-capture and dispatch-time recount.
| ): Promise<void> { | ||
| let toolTokens = 0; | ||
| const countedToolNames = new Set<string>(); | ||
| const rawToolTokenCounts: Record<string, number> = {}; |
There was a problem hiding this comment.
When an externally supplied tool name collides with an inherited object property such as toString, constructor, or __proto__, this plain object lookup is not numeric: (rawToolTokenCounts[name] ?? 0) + schemaTokens can produce a string/NaN or mutate the prototype, so the new per-tool counts in ON_CONTEXT_USAGE become corrupt or disappear. Use a Map or Object.create(null) for tool-name keyed accounting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d271673 — both per-tool accumulation maps are Object.create(null), so external tool names like toString/__proto__ can't hit inherited properties.
| ); | ||
| } | ||
| } | ||
| void safeDispatchCustomEvent( |
There was a problem hiding this comment.
For hosts that register an async ON_CONTEXT_USAGE handler (for example to persist the snapshot or update UI state), this fire-and-forget dispatch races with the immediately following model invocation and can still be running after deltas are emitted or even after a very fast/failing call returns. Other user-facing graph events in this path await safeDispatchCustomEvent; this one should do the same so the promised pre-invoke snapshot is delivered in order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d271673 — the dispatch is awaited, so async host handlers receive the pre-invoke snapshot before any model deltas.
| contextBudget, | ||
| effectiveInstructionTokens, |
There was a problem hiding this comment.
When summarization compacts away every message, the pruner's empty-message branch returns no contextBudget or effectiveInstructionTokens, but this node still invokes the model because hasPendingCompactionSummary() injects the summary through the system runnable. In that post-compaction call, ON_CONTEXT_USAGE is emitted with an undefined budget and a misleading full-window remainingContextTokens; provide the same reserve-adjusted budget fields for the empty-message branch before building the event.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d271673 — the pruner's empty-messages branch now returns the same reserve-adjusted contextBudget/effectiveInstructionTokens and an instruction-aware remaining, so post-compaction calls emit a coherent snapshot. Updated the empty-guard spec to the new semantics.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d271673db7
ℹ️ 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".
| : 1; | ||
| if ( | ||
| agentContext.tokenCounter != null && | ||
| finalMessages.length !== messagesToUse.length |
There was a problem hiding this comment.
When useLegacyContent is enabled, formatContentStrings() returns a new array with the same length and can mutate text-block content on any human/AI/system message, not just the trailing tool batch. Because this branch only does a full recount when the length changes, those same-length prompt rewrites fall through to the tail-delta path, so ON_CONTEXT_USAGE.remainingContextTokens can be computed from stale pre-format token counts for earlier messages even though the model receives the rewritten content.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining this one — the magnitude doesn't justify a full recount. formatContentStrings only flattens all-text-block arrays into a single string (joined with \n, trimmed); the token counter extracts and counts the same text either way (getTokenCountForMessage walks text blocks), so the per-message delta is tokenizer boundary/trim noise of ~0–2 tokens. Crucially, the pruner's own budget enforcement uses the same pre-format counts (indexTokenCountMap is built from pre-format content), so the snapshot is exactly as accurate as the budget math that decided what to send — there's no additional staleness introduced by the event. Tail-batch legacy conversion is captured: the tail snapshot is taken before formatContentStrings runs. Adding an O(n) full-array re-tokenization on every call for legacy-content providers would chase sub-token noise at real per-call cost.
| contextUsage.remainingContextTokens = Math.max( | ||
| 0, | ||
| contextUsage.contextBudget - | ||
| contextUsage.effectiveInstructionTokens - | ||
| Math.round(rawTokens * usageRatio) | ||
| ); |
There was a problem hiding this comment.
When post-prune formatting changes the message array length, such as formatArtifactPayload() appending a HumanMessage for OpenAI/Google tool artifacts or orphan sanitization dropping messages, this recount updates only remainingContextTokens. The nested breakdown.messageCount and breakdown.messageTokens were already frozen from the pre-format context, so the emitted ON_CONTEXT_USAGE snapshot can simultaneously report one token total in remainingContextTokens and a stale, smaller message count/token total in breakdown for the exact prompt being sent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d900eff. The dispatch-time adjustments now re-derive the budget-coupled breakdown fields via a shared syncBudgetDerivedFields helper: the full-recount branch updates breakdown.messageCount to finalMessages.length, and after either adjustment branch breakdown.messageTokens is recomputed from contextBudget - effectiveInstructionTokens - remainingContextTokens — the same identity used at snapshot construction — so the emitted breakdown always describes the payload actually sent.
| * budget math as `remainingContextTokens` (the index map is keyed | ||
| * by pre-prune state indices, so summing it over `context` would | ||
| * missum); `prePruneContextTokens` carries the pre-prune metric. */ | ||
| const usageBreakdown = agentContext.getTokenBudgetBreakdown(messages); |
There was a problem hiding this comment.
Once calibration has produced a resolvedInstructionOverhead, the pruner may use that value as effectiveInstructionTokens, but this snapshot still builds breakdown from agentContext.instructionTokens and only patches messageTokens. In calibrated runs where the effective overhead differs from the local estimate, breakdown.instructionTokens and breakdown.availableForMessages no longer add up with contextBudget/remainingContextTokens, so consumers of the new event get an internally inconsistent budget snapshot even before any formatting changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d900eff. syncBudgetDerivedFields now patches the aggregate breakdown.instructionTokens to the pruner's effectiveInstructionTokens and recomputes availableForMessages as contextBudget - effectiveInstructionTokens, so the breakdown reconciles with the top-level budget fields even when the calibration variance threshold leaves the local component estimates untouched. Component fields (system/dynamic/tool/summary) intentionally remain local estimates — the helper's doc comment notes this. Regression assertions added to context-usage-event.test.ts verifying instructionTokens === effectiveInstructionTokens and the additive identities.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d900eff3f8
ℹ️ 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".
| /** Same-length formatting can still grow the trailing tool batch | ||
| * in place — adjust remaining by its calibrated delta */ | ||
| let postFormatTailTokens = 0; | ||
| for (const message of finalMessages.slice(tailStart)) { |
There was a problem hiding this comment.
When useLegacyContent is enabled, formatContentStrings(finalMessages) walks the entire payload and mutates any human/ai/system message whose content is an all-text block array, not just the trailing tool batch. If one of those converted messages is earlier than tailStart and the array length is unchanged, this branch only recalculates finalMessages.slice(tailStart), so the emitted ON_CONTEXT_USAGE snapshot can report stale remainingContextTokens/messageTokens for the actual prompt sent to the model. This affects legacy-content providers whenever older kept messages contain text-block arrays.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Implemented a targeted version in 235ef31. Rather than a full O(n) re-tokenization per call, the dispatch path now tracks the exact set of pre-tail messages formatContentStrings will flatten — via a shared isLegacyConvertible predicate the formatter itself now uses, so the two can't drift — captures their pre-format counts, and folds their post-format delta into the same-length adjustment alongside the tail delta. In the common case (no all-text-block arrays before the tail) this adds zero tokenization; when convertible messages exist, only those messages are recounted and the snapshot is exact.
| const toolTokenCounts: Record<string, number> = Object.create(null); | ||
| const deferredToolNames: string[] = []; | ||
| for (const [name, rawCount] of Object.entries(rawToolTokenCounts)) { | ||
| toolTokenCounts[name] = Math.ceil(rawCount * toolTokenMultiplier); |
There was a problem hiding this comment.
For providers using the 1.4/2.6 tool multiplier, this rounds each tool independently while toolSchemaTokens is ceil(sum(raw) * multiplier). With multiple tools, summing toolTokenCounts can therefore exceed toolSchemaTokens by up to almost one token per tool, so consumers that use the new per-tool breakdown to explain or chart total tool overhead will get a total that does not match the snapshot's aggregate, especially in runs with many small/deferred tools.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 235ef31. Per-tool counts are now derived with largest-remainder apportionment (apportionTokenCounts) so they sum exactly to the aggregate toolSchemaTokens — applied both at initial counting and when calibration rescales the breakdown (the proportional-scaling path had the same drift). Unit tests cover ceil-of-sum aggregates, remainder priority, and calibration-style rescaling.
| await safeDispatchCustomEvent( | ||
| GraphEvents.ON_CONTEXT_USAGE, | ||
| contextUsage, | ||
| config |
There was a problem hiding this comment.
When a primary model invocation throws and fallbacks are configured, this emits ON_CONTEXT_USAGE before the failed primary attempt and then tryFallbackProviders can make a successful fallback call without emitting a fresh snapshot for that actual provider call. Hosts that correlate these snapshots with CHAT_MODEL_END usage (as the new tests do) will pair fallback usage with a primary-provider budget estimate, which is wrong when the fallback has a different tokenizer/tool overhead and also violates the advertised per-model-call semantics in fallback runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining the re-dispatch, with the semantics now documented on ContextUsageEvent (235ef31). Reasoning: (1) the fallback retry sends the byte-identical prompt — the snapshot describes that prompt, and the event intentionally carries no provider/model fields, so it isn't claiming fallback-provider numbers; (2) a genuinely correct fallback snapshot isn't computable with current plumbing — there is a single run-level tokenCounter (keyed to the primary model's encoding), so re-dispatching would emit the same numbers under a different label, which is misleading precision rather than accuracy; (3) the calibration ratio is fed by whichever provider actually reports usage, so fallback runs self-correct on subsequent calls. If per-fallback tokenizers land in the SDK someday, dispatching inside tryFallbackProviders would be the right follow-up.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…ibrate per-tool counts
…ispatch, compaction budget fields
- sync breakdown.instructionTokens/availableForMessages to the pruner's effective (calibrated) overhead so the aggregate agrees with the top-level budget fields even under the calibration variance threshold - re-derive breakdown.messageTokens and update messageCount after the dispatch-time recount/tail-delta adjustments so the snapshot describes the payload actually sent
- track the exact set of messages formatContentStrings will flatten via a shared isLegacyConvertible predicate and fold their token delta into the dispatch-time adjustment, so legacy-content rewrites before the trailing batch no longer skew the snapshot (zero extra tokenization when no convertible messages exist) - apportion per-tool schema counts with the largest-remainder method so they sum exactly to the aggregate, both at initial counting and when calibration rescales them - document fallback-retry snapshot semantics on ContextUsageEvent
235ef31 to
ba3fc3d
Compare
1 participant
Summary
I added a first-class
ON_CONTEXT_USAGEevent so hosts can stream per-model-call context window accounting to their UIs (LibreChat consumes this for its context usage gauge in LibreChat-AI/LibreChat#13670).ON_CONTEXT_USAGEfromcreateCallModelimmediately after pruning, viasafeDispatchCustomEvent(same fire-and-forget mechanism asON_AGENT_LOG); fires on every model call, including tool-loop iterations, and only when atokenCounter/maxContextTokenspruner exists.TokenBudgetBreakdownfromAgentContext.getTokenBudgetBreakdownplus two fields the pruner already knows but did not return:contextBudget(maxTokens minus the output reserve) andeffectiveInstructionTokens(the calibrated instruction overhead actually applied this call) — so consumers derive post-prune occupancy ascontextBudget − remainingContextTokenswith zero duplicated math. The breakdown's index-keyedmessageTokensis pre-prune by design; the payload documents this.ContextUsageEventtype and register the event inCUSTOM_GRAPH_EVENTSso the run loop routes it to host handler registries.Testing
src/specs/context-usage-event.test.ts: asserts one snapshot per model call with budget invariants through a realRun.create+processStream, and that no event fires without atokenCounter.prune,token-accounting(pipeline + e2e),thinking-prune, andsummarize-prunesuites pass (111 tests) alongside the new spec;tsc --noEmitclean;npm run buildclean.