fix(runner): stop reporting context-window size as a run's token usage - #5710
fix(runner): stop reporting context-window size as a run's token usage#5710mmabrouk wants to merge 1 commit into
Conversation
dosubot
Bot
added
the
size:M
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This is one of three independent fixes for the same reported problem: an agent run shows a cost in the playground and nothing in the trace. They can be reviewed and merged separately, in any order.
Live end-to-end verification of all three together on a running stack is in progress, and I will post the result here. @coderabbitai review |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change separates ACP context occupancy from run token usage. It preserves reported stream and harness costs, including zero, omits unknown costs, and skips empty OpenTelemetry usage updates. Unit tests cover sandbox usage resolution and telemetry stamping. ChangesUsage Corrections
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== PR metadata and changed files =="
gh pr view 5710 --repo Agenta-AI/agenta --json title,baseRefName,headRefName,files,body \
--jq '{title, baseRefName, headRefName, files: [.files[].path]}'
echo
echo "== Changed-file diff summary =="
git diff --stat "origin/$(gh pr view 5710 --repo Agenta-AI/agenta --json baseRefName --jq -r .baseRefName)"...HEAD
echo
echo "== Changed-file names =="
git diff --name-only "origin/$(gh pr view 5710 --repo Agenta-AI/agenta --json baseRefName --jq -r .baseRefName)"...HEAD
echo
echo "== Usage-related symbols and nearby definitions =="
rg -n -C 5 -g '*.{ts,tsx}' \
'mergePromptAndStreamUsage|stampUsage|usage_update|usageUpdate|PromptResponse|setUsage' .Length of output: 1717 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== PR diff from GitHub =="
gh pr diff 5710 --repo Agenta-AI/agenta --patch
echo
echo "== Modified source-file outlines =="
ast-grep outline services/runner/src/engines/sandbox_agent/usage.ts --items all
ast-grep outline services/runner/src/tracing/otel.ts --items all
echo
echo "== Modified source files =="
cat -n services/runner/src/engines/sandbox_agent/usage.ts
cat -n services/runner/src/tracing/otel.ts
echo
echo "== Modified tests =="
cat -n services/runner/tests/unit/otel-usage-context-size.test.ts
cat -n services/runner/tests/unit/sandbox-agent-usage.test.ts
echo
echo "== Direct usage-accounting call sites and ACP usage-update definitions =="
rg -n -C 4 -g '*.{ts,tsx}' \
'mergePromptAndStreamUsage\s*\(|stampUsage\s*\(|usage_update|used\s*:' \
services/runner/src services/runner/testsLength of output: 139398 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== AgentUsage and usage event contract =="
rg -n -C 8 -g '*.{ts,tsx}' \
'interface AgentUsage|type AgentUsage|type: "usage"|interface .*Usage' \
services/runner/src/protocol.ts services/runner/src
echo
echo "== Usage resolution and tracer writeback call sites =="
rg -n -C 10 -g '*.{ts,tsx}' \
'resolveRunUsage\s*\(|\.setUsage\s*\(|otel\.usage\s*\(' \
services/runner/src services/runner/tests
echo
echo "== Existing ACP stream-event assertions =="
cat -n services/runner/tests/unit/stream-events.test.ts
echo
echo "== Dependency declarations and ACP type definitions, if checked in =="
rg -n -C 5 -g '*.{ts,json,yaml,yml}' \
'agent-client-protocol|`@agentclientprotocol`|usage_update|UsageUpdate' \
services/runner/package.json services/runner/pnpm-lock.yaml package.json pnpm-lock.yaml 2>/dev/null || true
fd -HI -t f '.*\.(ts|d\.ts)$' . | rg 'agent.*protocol|acp' | head -80Length of output: 53953
The change removes ACP I did not run the test suite. The review sandbox does not permit execution of repository code. ✏️ Learnings added
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fb38a46a-63cb-4a92-8c46-01fe6b6ddd65
📒 Files selected for processing (4)
services/runner/src/engines/sandbox_agent/usage.tsservices/runner/src/tracing/otel.tsservices/runner/tests/unit/otel-usage-context-size.test.tsservices/runner/tests/unit/sandbox-agent-usage.test.ts
| const cost = update.cost?.amount; | ||
| const total = update.used; | ||
| if (typeof cost !== "number") return; | ||
| usage = { | ||
| input: usage?.input ?? 0, | ||
| output: usage?.output ?? 0, | ||
| total: typeof total === "number" ? total : usage?.total ?? 0, | ||
| cost: typeof cost === "number" ? cost : usage?.cost ?? 0, | ||
| total: usage?.total ?? 0, | ||
| cost, | ||
| }; | ||
| record({ type: "usage", ...usage }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Skip all-zero usage_update records.
When cost.amount is 0 and no token split exists, Line 1458 accepts the cost and Lines 1459-1465 emit an all-zero usage event. This contradicts the requirement to skip measurements with neither tokens nor cost.
Build the next usage value first. Record it only when its total tokens or cost is positive. Add a regression test for contextSizeUpdate(63369, 0).
Proposed fix
const cost = update.cost?.amount;
if (typeof cost !== "number") return;
-usage = {
+const nextUsage = {
input: usage?.input ?? 0,
output: usage?.output ?? 0,
total: usage?.total ?? 0,
cost,
};
+if (nextUsage.total <= 0 && nextUsage.cost <= 0) return;
+usage = nextUsage;
record({ type: "usage", ...usage });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const cost = update.cost?.amount; | |
| const total = update.used; | |
| if (typeof cost !== "number") return; | |
| usage = { | |
| input: usage?.input ?? 0, | |
| output: usage?.output ?? 0, | |
| total: typeof total === "number" ? total : usage?.total ?? 0, | |
| cost: typeof cost === "number" ? cost : usage?.cost ?? 0, | |
| total: usage?.total ?? 0, | |
| cost, | |
| }; | |
| record({ type: "usage", ...usage }); | |
| const cost = update.cost?.amount; | |
| if (typeof cost !== "number") return; | |
| const nextUsage = { | |
| input: usage?.input ?? 0, | |
| output: usage?.output ?? 0, | |
| total: usage?.total ?? 0, | |
| cost, | |
| }; | |
| if (nextUsage.total <= 0 && nextUsage.cost <= 0) return; | |
| usage = nextUsage; | |
| record({ type: "usage", ...usage }); |
Railway Preview Environment
|
When the harness's PromptResponse carried no usage, the runner fell back to the ACP stream's `usage_update.used` value and reported it as the run's token total. That value is not a token count of the run. It is how full the agent's context window is at that point in the turn. Measured on one stack: 6,041 root spans shaped `input 0 / output 0 / total 63369`, with no cost. A wrong number here is worse than no number, because a reader cannot tell a real 63,369-token run from a context-size artifact, and any token aggregate built on that path is silently poisoned. Fixing the usage merge alone would not have removed those rows. The tracer keeps its own usage record built straight from the stream, and handing it nothing is a no-op rather than a clear, so it kept stamping the context size on the root span anyway. So the stream handler now drops `used` and keeps only the reported cost, and an update carrying neither is not recorded at all. The stamping function now also skips a usage record with no tokens and no cost. All zeros is the absence of a measurement, not a measured zero, and asserting that a run spent nothing is its own wrong answer. Nothing consumes the context size. The only readers of the merged usage object are the span attributes and the Python SDK's `record_usage`, and both treat every number in it as a token count. Tests: the full runner suite passes, 1,486 tests across 97 files, plus nine new cases covering a context-size-only stream, a cost-only stream, a half-reported split, and a normal split delivered through the harness. Claude-Session: https://claude.ai/code/session_01RkWWQUNNzRbaB5jnCAdjYA
e3cf8b3 to
ab1460e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 663f547b-687b-4634-ac15-c378b101fc5f
📒 Files selected for processing (5)
services/runner/src/engines/sandbox_agent/usage.tsservices/runner/src/protocol.tsservices/runner/src/tracing/otel.tsservices/runner/tests/unit/otel-usage-context-size.test.tsservices/runner/tests/unit/sandbox-agent-usage.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- services/runner/src/engines/sandbox_agent/usage.ts
- services/runner/tests/unit/sandbox-agent-usage.test.ts
- services/runner/src/tracing/otel.ts
| /** | ||
| * INVARIANT: absent means the cost is UNKNOWN (the harness reported none); a present `0` is a | ||
| * measured zero — a free model or a fully cached turn. Consumers read presence as evidence of | ||
| * a measurement, so a producer must never substitute a zero for an absence: doing so records | ||
| * an unpriced run as a free one, which every downstream aggregate then believes. | ||
| */ | ||
| cost?: number; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate files"
fd -a 'protocol\.ts|otel\.ts' . | sed 's#^\./##'
echo
echo "protocol.ts cost lines"
cat -n services/runner/src/protocol.ts | sed -n '420,440p'
echo
echo "otel.ts stampUsage area"
wc -l services/runner/src/tracing/otel.ts
cat -n services/runner/src/tracing/otel.ts | sed -n '1130,1205p'
echo
echo "Search stampUsage and cost references"
rg -n "stampUsage|cost" services/runner/src || trueRepository: Agenta-AI/agenta
Length of output: 9875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Behavioral probe of the relevant guard expression for absent cost.
node - <<'JS'
function guard(cost, total) {
return cost <= 0 && total <= 0;
}
const cases = [
{ cost: undefined, total: 0 },
{ cost: 0, total: 0 },
{ cost: 1, total: 0 },
{ cost: undefined, total: undefined },
];
for (const c of cases) {
console.log(JSON.stringify({
cost: c.cost,
total: c.total,
originalGuard: guard(c.cost, c.total),
normalizedCostGuard: (c.cost ?? 0) <= 0 && c.total <= 0,
}));
}
JSRepository: Agenta-AI/agenta
Length of output: 407
Normalize absent cost in stampUsage.
stampUsage checks u.total <= 0 && u.cost <= 0, but u.cost is optional. For { input: 0, output: 0, total: 0 } with no cost, undefined <= 0 is false, so the guard falls through and writes zero token attributes. Use const cost = u.cost ?? 0 for the empty-record guard, and keep the u.cost > 0 check when stamping the gen_ai.usage.cost attribute. Add a regression case for { input: 0, output: 0, total: 0 } with no cost.
Source: Coding guidelines
1 participant
The symptom
Agent runs report token totals that are not token totals.
Measured on one stack: 6,041 root spans shaped
input 0 / output 0 / total 63369, with no cost.The cause
When the harness's
PromptResponsecarries no usage, the runner fell back to the ACP stream'susage_update.usedvalue and reported it as the run's token total.That value is not a count of the tokens the run spent. It is how full the agent's context window is at that point in the turn.
A wrong number here is worse than no number. A reader cannot tell a genuine 63,369-token run from a context-size artifact, and any token aggregate built on that path is silently poisoned.
The fix
Fixing the usage merge alone would not have removed those rows. The tracer keeps its own usage record built straight from the stream, and handing it
undefinedis a no-op rather than a clear, so it kept stamping the context size on the root span anyway.So the fix cuts at the source:
usage_updatehandler dropsusedentirely and keeps only the reported cost. An update carrying neither is not recorded.mergePromptAndStreamUsagederives the total solely from the harness's split. Cost-only usage is still returned, as before.stampUsageskips a usage record with no tokens and no cost. All zeros is the absence of a measurement, not a measured zero, and asserting that a run cost nothing is its own wrong answer.Nothing about what the harness reports changed, and the cost path is untouched.
Why dropping the value is safe
Every consumer of the merged usage object was traced:
run.setUsageundefined, stamps nothingrecord_usagewire.pydata.get("usage"), no presence assumptiontranscriptToMessages!== undefinedThe other tracer in
src/extensions/agenta.tsis fed by Pi's own token counts, not the ACP path, and is untouched.No consumer wants the context size, so carrying it under a distinct field would have added a wire field to the mirrored
protocol.tsandwire.pycontract with zero subscribers. Absent data reading as absent is the honest option.Verification
tsc --noEmitclean.usage_updatecarrying onlyusedproduces no usage event and nogen_ai.usage.*on the root span.Related
Part of a set of three independent fixes for the same reported problem, that an agent run shows a cost in the playground and none in the trace. The other two are the Python SDK streaming fix and the API ingest fix.