fix(core): tighten prompt context usage telemetry#1172
Merged
Conversation
🦋 Changeset detectedLatest commit: 0ece01d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This comment has been minimized.
This comment has been minimized.
Contributor
📝 WalkthroughWalkthroughThis PR refines telemetry collection for prompt-context usage and token attributes by implementing circular reference protection in serialization, redacting binary fields during token estimation, and only emitting cached/reasoning token attributes when their values exceed zero. Corresponding tests validate the updated sanitization and observability behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/agent/prompt-context-usage.spec.ts">
<violation number="1" location="packages/core/src/agent/prompt-context-usage.spec.ts:151">
P2: This equality-only assertion can pass when both estimates are undefined, so the test may miss regressions in token estimation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }, | ||
| }); | ||
|
|
||
| expect(withArrayArgs?.toolTokensEstimated).toBe(withoutArgs?.toolTokensEstimated); |
Contributor
There was a problem hiding this comment.
P2: This equality-only assertion can pass when both estimates are undefined, so the test may miss regressions in token estimation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/agent/prompt-context-usage.spec.ts, line 151:
<comment>This equality-only assertion can pass when both estimates are undefined, so the test may miss regressions in token estimation.</comment>
<file context>
@@ -68,4 +68,86 @@ describe("prompt context usage estimation", () => {
+ },
+ });
+
+ expect(withArrayArgs?.toolTokensEstimated).toBe(withoutArgs?.toolTokensEstimated);
+ });
});
</file context>
Suggested change
| expect(withArrayArgs?.toolTokensEstimated).toBe(withoutArgs?.toolTokensEstimated); | |
| expect(withArrayArgs?.toolTokensEstimated).toBeGreaterThan(0); | |
| expect(withoutArgs?.toolTokensEstimated).toBeGreaterThan(0); | |
| expect(withArrayArgs?.toolTokensEstimated).toBe(withoutArgs?.toolTokensEstimated); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/agent/prompt-context-usage.spec.ts (1)
72-129: Split this case to isolate failures faster.This test currently changes nested binary size,
providerOptions, andneedsApprovaltogether. Splitting into focused assertions will make regressions easier to pinpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/agent/prompt-context-usage.spec.ts` around lines 72 - 129, The test "sanitizes nested binary args recursively and ignores provider-only metadata" mixes three variables (nested binary sizes in circularArgsA/B, providerOptions.openai.metadata, and needsApproval) which obscures failures; split this into multiple focused tests that each change only one factor: 1) a test that verifies sanitization of large nested binary data using circularArgsA vs circularArgsB while keeping providerOptions and needsApproval identical, 2) a test that verifies provider-only metadata in providerOptions.openai is ignored by swapping only the metadata value while keeping args and needsApproval identical, and 3) a test that verifies needsApproval does not affect token estimation by toggling needsApproval while keeping args and providerOptions identical; use the existing estimatePromptContextUsage call and the tool key searchDocs (and circularArgsA/circularArgsB) to locate and implement each isolated assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/agent/agent-observability.spec.ts`:
- Around line 216-219: Replace the untyped usage of events with the exported
ObservabilityWebSocketEvent type: add ObservabilityWebSocketEvent to the
existing import from "../observability" (alongside NodeVoltAgentObservability
and WebSocketEventEmitter), change the declaration const events: any[] to const
events: ObservabilityWebSocketEvent[], and type the callback parameter in
WebSocketEventEmitter.getInstance().onWebSocketEvent((event) => ...) to (event:
ObservabilityWebSocketEvent) => { ... } ; apply the same replacement for any
other occurrences in this test file to restore TypeScript type-safety.
In `@packages/core/src/agent/prompt-context-usage.ts`:
- Around line 168-186: sanitizeRecordValue currently recurses into nested
content via serializePromptValue(record.content) without passing the seen Set,
which allows circular structures (e.g., a.content = b; b.content = a) to recurse
infinitely; fix by threading the seen set through the recursion — either call
sanitizeValue(record.content, seen) instead of
serializePromptValue(record.content) from inside sanitizeRecordValue, or extend
serializePromptValue to accept a seen: Set<object> parameter and pass it along
from sanitizeRecordValue so circular detection is preserved (update related
callers accordingly).
---
Nitpick comments:
In `@packages/core/src/agent/prompt-context-usage.spec.ts`:
- Around line 72-129: The test "sanitizes nested binary args recursively and
ignores provider-only metadata" mixes three variables (nested binary sizes in
circularArgsA/B, providerOptions.openai.metadata, and needsApproval) which
obscures failures; split this into multiple focused tests that each change only
one factor: 1) a test that verifies sanitization of large nested binary data
using circularArgsA vs circularArgsB while keeping providerOptions and
needsApproval identical, 2) a test that verifies provider-only metadata in
providerOptions.openai is ignored by swapping only the metadata value while
keeping args and needsApproval identical, and 3) a test that verifies
needsApproval does not affect token estimation by toggling needsApproval while
keeping args and providerOptions identical; use the existing
estimatePromptContextUsage call and the tool key searchDocs (and
circularArgsA/circularArgsB) to locate and implement each isolated assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3f34a3d-3d5a-4e52-a26e-7b7f8269f5c4
📒 Files selected for processing (5)
.changeset/four-rules-peel.mdpackages/core/src/agent/agent-observability.spec.tspackages/core/src/agent/agent.tspackages/core/src/agent/prompt-context-usage.spec.tspackages/core/src/agent/prompt-context-usage.ts
Comment on lines
+216
to
+219
| const events: any[] = []; | ||
| const unsubscribe = WebSocketEventEmitter.getInstance().onWebSocketEvent((event) => { | ||
| events.push(event); | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify explicit any usage in TypeScript test files.
rg -nP --type=ts '\bevents:\s*any\[\]' -C2Repository: VoltAgent/voltagent
Length of output: 5900
🏁 Script executed:
#!/bin/bash
# Find WebSocketEventEmitter definition and its onWebSocketEvent callback signature
rg -n 'onWebSocketEvent\s*\(' -A3 packages/core/src/agent/agent-observability.spec.ts | head -20Repository: VoltAgent/voltagent
Length of output: 658
🏁 Script executed:
#!/bin/bash
# Search for WebSocketEventEmitter class definition
rg -n 'class WebSocketEventEmitter' -A20Repository: VoltAgent/voltagent
Length of output: 2286
🏁 Script executed:
#!/bin/bash
# Find the type/interface for events in WebSocketEventEmitter
fd -t f -e ts -e tsx | xargs rg -l 'WebSocketEventEmitter' | head -5Repository: VoltAgent/voltagent
Length of output: 313
🏁 Script executed:
#!/bin/bash
# Find ObservabilityWebSocketEvent type definition
rg -n 'type ObservabilityWebSocketEvent|interface ObservabilityWebSocketEvent' -A10Repository: VoltAgent/voltagent
Length of output: 843
🏁 Script executed:
#!/bin/bash
# Check if ObservabilityWebSocketEvent is exported and available for import
rg -n 'export.*ObservabilityWebSocketEvent' -B2 -A2Repository: VoltAgent/voltagent
Length of output: 460
🏁 Script executed:
#!/bin/bash
# Check what imports are currently in the test file
head -30 packages/core/src/agent/agent-observability.spec.tsRepository: VoltAgent/voltagent
Length of output: 957
🏁 Script executed:
#!/bin/bash
# Verify if ObservabilityWebSocketEvent is exported from observability/index.ts or main index
rg -n 'export.*ObservabilityWebSocketEvent' packages/core/src/observability/index.ts packages/core/src/index.tsRepository: VoltAgent/voltagent
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check what's exported from observability index
cat packages/core/src/observability/index.ts | head -50Repository: VoltAgent/voltagent
Length of output: 2179
🏁 Script executed:
#!/bin/bash
# Check if ObservabilityWebSocketEvent is available in observability exports
rg 'export' packages/core/src/observability/index.ts | grep -i websocketRepository: VoltAgent/voltagent
Length of output: 151
🏁 Script executed:
#!/bin/bash
# Get full exports from observability/index.ts
rg '^export' packages/core/src/observability/index.tsRepository: VoltAgent/voltagent
Length of output: 913
🏁 Script executed:
#!/bin/bash
# Check where WebSocketEventEmitter is exported from
rg -n 'export.*WebSocketEventEmitter' packages/core/src/observability/Repository: VoltAgent/voltagent
Length of output: 200
🏁 Script executed:
#!/bin/bash
# Get the complete export statements from observability index
cat packages/core/src/observability/index.ts | grep -A5 'export {'Repository: VoltAgent/voltagent
Length of output: 1258
🏁 Script executed:
#!/bin/bash
# Check what's exported that contains WebSocketEventEmitter or if it's only exported from processors
rg 'WebSocketEventEmitter' packages/core/src/observability/index.tsRepository: VoltAgent/voltagent
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Check the import statement in the test to see where WebSocketEventEmitter comes from
grep 'WebSocketEventEmitter' packages/core/src/agent/agent-observability.spec.tsRepository: VoltAgent/voltagent
Length of output: 1160
🏁 Script executed:
#!/bin/bash
# Check if ObservabilityWebSocketEvent is exported from observability/index.ts
rg 'ObservabilityWebSocketEvent' packages/core/src/observability/index.tsRepository: VoltAgent/voltagent
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Get all type exports from observability/index.ts
rg 'export type' packages/core/src/observability/index.tsRepository: VoltAgent/voltagent
Length of output: 229
🏁 Script executed:
#!/bin/bash
# Get the complete import statement from the test file
head -10 packages/core/src/agent/agent-observability.spec.ts | grep -A5 'import.*observability'Repository: VoltAgent/voltagent
Length of output: 334
🏁 Script executed:
#!/bin/bash
# Check the exact lines 216-219 and surrounding context
sed -n '214,225p' packages/core/src/agent/agent-observability.spec.tsRepository: VoltAgent/voltagent
Length of output: 527
Import and use ObservabilityWebSocketEvent to maintain type safety.
Line 216 uses any[], which defeats TypeScript's compile-time safety. Replace with the proper event type already exported from the observability module:
-
Add
ObservabilityWebSocketEventto the import from"../observability":import { NodeVoltAgentObservability, WebSocketEventEmitter, ObservabilityWebSocketEvent } from "../observability";
-
Type the
eventsarray and callback parameter:const events: ObservabilityWebSocketEvent[] = []; const unsubscribe = WebSocketEventEmitter.getInstance().onWebSocketEvent((event: ObservabilityWebSocketEvent) => { events.push(event); });
The ObservabilityWebSocketEvent interface is already defined and exported; no need for a local type definition. This applies to all similar instances in this test file.
As per coding guidelines: **/*.ts — Maintain type safety in TypeScript-first codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/agent/agent-observability.spec.ts` around lines 216 - 219,
Replace the untyped usage of events with the exported
ObservabilityWebSocketEvent type: add ObservabilityWebSocketEvent to the
existing import from "../observability" (alongside NodeVoltAgentObservability
and WebSocketEventEmitter), change the declaration const events: any[] to const
events: ObservabilityWebSocketEvent[], and type the callback parameter in
WebSocketEventEmitter.getInstance().onWebSocketEvent((event) => ...) to (event:
ObservabilityWebSocketEvent) => { ... } ; apply the same replacement for any
other occurrences in this test file to restore TypeScript type-safety.
Comment on lines
+168
to
+186
| return sanitizeRecordValue(record, new Set<object>()); | ||
| } | ||
|
|
||
| function sanitizeRecordValue( | ||
| record: Record<string, unknown>, | ||
| seen: Set<object>, | ||
| ): Record<string, unknown> { | ||
| if (seen.has(record)) { | ||
| return { circular: CIRCULAR_REFERENCE_PLACEHOLDER }; | ||
| } | ||
|
|
||
| seen.add(record); | ||
| const sanitized: Record<string, unknown> = {}; | ||
|
|
||
| for (const [key, value] of Object.entries(record)) { | ||
| sanitized[key] = LARGE_BINARY_KEYS.has(key) ? "[omitted]" : value; | ||
| sanitized[key] = LARGE_BINARY_KEYS.has(key) ? "[omitted]" : sanitizeValue(value, seen); | ||
| } | ||
|
|
||
| seen.delete(record); |
Contributor
There was a problem hiding this comment.
Circular protection still misses recursive content traversal.
Line 157 recursively calls serializePromptValue(record.content) without seen tracking, so payloads like a.content = b; b.content = a can still hit infinite recursion before the sanitizer fallback executes.
Proposed fix
-function serializePromptValue(value: unknown): string {
+function serializePromptValue(value: unknown, seen: Set<object> = new Set()): string {
if (typeof value === "string") {
return value;
}
if (typeof value === "number" || typeof value === "boolean") {
return String(value);
}
if (Array.isArray(value)) {
- return value
- .map((entry) => serializePromptValue(entry))
- .filter((entry) => entry.trim().length > 0)
- .join("\n");
+ if (seen.has(value)) return CIRCULAR_REFERENCE_PLACEHOLDER;
+ seen.add(value);
+ try {
+ return value
+ .map((entry) => serializePromptValue(entry, seen))
+ .filter((entry) => entry.trim().length > 0)
+ .join("\n");
+ } finally {
+ seen.delete(value);
+ }
}
if (!value || typeof value !== "object") {
return "";
}
+ if (seen.has(value as object)) return CIRCULAR_REFERENCE_PLACEHOLDER;
+ seen.add(value as object);
- const record = value as Record<string, unknown>;
- const type = typeof record.type === "string" ? record.type : undefined;
+ try {
+ const record = value as Record<string, unknown>;
+ const type = typeof record.type === "string" ? record.type : undefined;
- if (typeof record.text === "string") {
- return record.text;
- }
+ if (typeof record.text === "string") {
+ return record.text;
+ }
- if (type && BINARY_PART_TYPES.has(type)) {
- return `[${type}]`;
- }
+ if (type && BINARY_PART_TYPES.has(type)) {
+ return `[${type}]`;
+ }
- if (type === "tool-call") {
- const toolName = typeof record.toolName === "string" ? record.toolName : "tool";
- const input = serializePromptValue(record.input);
- return input ? `tool-call ${toolName}: ${input}` : `tool-call ${toolName}`;
- }
+ if (type === "tool-call") {
+ const toolName = typeof record.toolName === "string" ? record.toolName : "tool";
+ const input = serializePromptValue(record.input, seen);
+ return input ? `tool-call ${toolName}: ${input}` : `tool-call ${toolName}`;
+ }
- if (type === "tool-result") {
- const toolName = typeof record.toolName === "string" ? record.toolName : "tool";
- const output = serializePromptValue(record.output);
- return output ? `tool-result ${toolName}: ${output}` : `tool-result ${toolName}`;
- }
+ if (type === "tool-result") {
+ const toolName = typeof record.toolName === "string" ? record.toolName : "tool";
+ const output = serializePromptValue(record.output, seen);
+ return output ? `tool-result ${toolName}: ${output}` : `tool-result ${toolName}`;
+ }
- if ("content" in record) {
- const nestedContent = serializePromptValue(record.content);
- if (nestedContent) {
- return nestedContent;
+ if ("content" in record) {
+ const nestedContent = serializePromptValue(record.content, seen);
+ if (nestedContent) {
+ return nestedContent;
+ }
}
- }
- return safeStringify(sanitizeRecord(record));
+ return safeStringify(sanitizeRecord(record));
+ } finally {
+ seen.delete(value as object);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/agent/prompt-context-usage.ts` around lines 168 - 186,
sanitizeRecordValue currently recurses into nested content via
serializePromptValue(record.content) without passing the seen Set, which allows
circular structures (e.g., a.content = b; b.content = a) to recurse infinitely;
fix by threading the seen set through the recursion — either call
sanitizeValue(record.content, seen) instead of
serializePromptValue(record.content) from inside sanitizeRecordValue, or extend
serializePromptValue to accept a seen: Set<object> parameter and pass it along
from sanitizeRecordValue so circular detection is preserved (update related
callers accordingly).
Deploying voltagent with
|
| Latest commit: |
0ece01d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6c499700.voltagent.pages.dev |
| Branch Preview URL: | https://fix-prompt-context-telemetry.voltagent.pages.dev |
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
1 participant
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.
PR Checklist
Please check if your PR fulfills the following requirements:
Bugs / Features
What is the current behavior?
Prompt-context token estimation can miss nested large-binary fields, include runtime-only tool metadata that inflates schema estimates, and emit zero-value cached/reasoning usage attributes even when providers did not report them.
What is the new behavior?
fixes (issue)
N/A
Notes for reviewers
Added targeted regression tests for nested sanitization, tool serialization filtering, and zero-value usage attribute emission.
Summary by cubic
Tighten prompt-context usage telemetry to improve token estimates and reduce noisy span attributes. More accurate metrics and cleaner observability.
args.cached_tokensandreasoning_tokensspan attributes only when values are > 0.Written for commit 0ece01d. Summary will update on new commits.
Summary by CodeRabbit