fix(#6904): parse thinking tokens from Claude stream events - #6907
Conversation
|
🤖 Finished Review · ✅ Success · Started 10:51 AM UTC · Completed 11:08 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.65 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Risk Assessment: moderate (2/5) DetailsWell-scoped bug fix (2 files, 127 lines) adding thinking-token parsing with strong test coverage (0.50 test ratio). Tier 2 shows continued fix churn on these files which keeps the score at moderate rather than low. Anchored to prior score=2 — no material signal changes warrant adjustment. Previous runRisk Assessment: moderate (2/5) DetailsWell-scoped bug fix (2 files, 127 lines) adding thinking-token parsing with strong test coverage (0.50 test ratio). Tier 2 shows continued fix churn on these files which keeps the score at moderate rather than low. Anchored to prior score=2 — no material signal changes warrant adjustment. Previous run (2)Risk Assessment: moderate (2/5) DetailsWell-scoped bug fix adding thinking-token parsing with strong test coverage; elevated slightly from low to moderate due to active fix churn on these files in recent weeks. |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsLow
Previous run (2)ReviewFindingsLow
Next steps:
Previous run (3)ReviewFindingsMedium
Low
Next steps:
|
|
🤖 Finished Fix · ✅ Success · Started 11:10 AM UTC · Completed 11:18 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.42 |
🔧 Fix agent — iteration 1 (bot-triggered)Renamed totalReasoning to msgReasoning (per-message) and accReasoning to totalReasoning (cross-message accumulator) to match pi_progress.go/opencode_progress.go conventions. Added msgReasoning to the throttle threshold calculation for consistency with the renderer. All tests pass. Fixed (3):
Tests: passed Decision points
Next steps:
|
Rename totalReasoning → msgReasoning (per-message, reset on message_start) and accReasoning → totalReasoning (cross-message accumulator) to match the naming convention in pi_progress.go and opencode_progress.go. Include msgReasoning in the throttle threshold so TokensEvent emissions are consistent with the renderer's total. Addresses #6907
|
🤖 Finished Review · ✅ Success · Started 11:20 AM UTC · Completed 11:34 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.38 |
|
🤖 Finished Fix · ✅ Success · Started 11:35 AM UTC · Completed 11:43 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.42 |
The comment in TestParseClaudeStreamTokensEventWithReasoningTokens incorrectly stated the total as 5700 (omitting the 300 reasoning tokens from the sum). The actual threshold calculation includes msgReasoning: 4000 + 1000 + 300 + 500 + 200 = 6000. Addresses #6907
🔧 Fix agent — iteration 2 (bot-triggered)Fixed the incorrect threshold comment in TestParseClaudeStreamTokensEventWithReasoningTokens to include the 300 reasoning tokens in the sum (6000, not 5700). Acknowledged the defensive coding consideration about potential double-counting in multi-delta scenarios but disagreed on making changes since the reviewer confirmed it does not manifest under the documented API contract. Fixed (1):
Disagreed (1):
Tests: passed Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 11:45 AM UTC · Completed 11:58 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.11 |
Superseded by updated review
|
/fs-fix rebase and solve the conflicts |
|
🤖 Finished Fix · ✅ Success · Started 11:04 AM UTC · Completed 11:16 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.23 |
🔧 Fix agent — iteration 3 (human-triggered)Rebased PR branch onto origin/main, resolving 2 conflict regions across 2 commits in internal/runtime/claude_progress.go. The conflicts arose from main's cumulative token tracking (#6905) overlapping with the PR's reasoning token parsing (#6904). Both features are now correctly merged. All tests pass, go vet passes, no secrets detected. Fixed (1):
Tests: passed Decision points
Next steps:
|
Rename totalReasoning → msgReasoning (per-message, reset on message_start) and accReasoning → totalReasoning (cross-message accumulator) to match the naming convention in pi_progress.go and opencode_progress.go. Include msgReasoning in the throttle threshold so TokensEvent emissions are consistent with the renderer's total. Addresses #6907
The comment in TestParseClaudeStreamTokensEventWithReasoningTokens incorrectly stated the total as 5700 (omitting the 300 reasoning tokens from the sum). The actual threshold calculation includes msgReasoning: 4000 + 1000 + 300 + 500 + 200 = 6000. Addresses #6907
d242248 to
021367a
Compare
|
🤖 Review · Commit: |
The Claude runtime parser (claude_progress.go) never populated ReasoningTokens because the message_delta handler only parsed usage.output_tokens, ignoring output_tokens_details.thinking_tokens from the Anthropic API. Add parsing of output_tokens_details.thinking_tokens from message_delta events and accumulate across turns. The per-message value is emitted on TokensEvent for progress display, and the accumulated total is emitted on ResultEvent so gen_ai.usage. reasoning_tokens appears on OTel spans for Claude Code runs. This closes the observability gap where Pi runs reported reasoning tokens but Claude Code runs always showed 0. Closes #6904
Rename totalReasoning → msgReasoning (per-message, reset on message_start) and accReasoning → totalReasoning (cross-message accumulator) to match the naming convention in pi_progress.go and opencode_progress.go. Include msgReasoning in the throttle threshold so TokensEvent emissions are consistent with the renderer's total. Addresses #6907
The comment in TestParseClaudeStreamTokensEventWithReasoningTokens incorrectly stated the total as 5700 (omitting the 300 reasoning tokens from the sum). The actual threshold calculation includes msgReasoning: 4000 + 1000 + 300 + 500 + 200 = 6000. Addresses #6907
021367a to
c3b88f4
Compare
|
🤖 Finished Review · ✅ Success · Started 11:22 AM UTC · Completed 12:03 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.38 |
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
internal/runtime/claude_progress.go(file-level): Line 147 · [medium] error-handling-gap
The deferred final TokensEvent (emitted when a stream ends without a ResultEvent, e.g. cancelled runs) does not include ReasoningTokens. The PR adds reasoning-token parsing to the message_delta path and to ResultEvent, but the deferred fallback omits it. The total calculation also omits reasoning, making the threshold check inconsistent with the message_delta handler's total. Note: the deferred path predates this PR (added by #6905) and is outside the issue's authorized scope, but totalReasoning is in scope and the fix is trivial.
Suggested fix: Add ReasoningTokens: totalReasoning to the deferred TokensEvent and include totalReasoning in the total calculation.
|
🤖 Finished Fix · ❌ Failure (post-script /home/runner/work/fullsend/fullsend/.fullsend/.fullsend-cache/resources/sha256/21644e0238d0b83db763fa138c412322223b0ba4a919a542b839e11b6a3d62e7/scripts/post-fix.sh failed: exit status 1) · Started 12:05 PM UTC · Completed 12:16 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.73 |
|
The fix agent completed, but the post-fix script failed before finishing. Workflow run: https://github.com/fullsend-ai/fullsend/actions/runs/33870891396 Details: |
|
🤖 Finished Retro · ✅ Success · Started 12:25 PM UTC · Completed 12:39 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.37 |
Retro: PR #6907 — parse thinking tokens from Claude stream eventsOverall: The initial code-review-fix cycle was productive and efficient. The code agent produced a well-scoped implementation, and the review agent caught genuine issues (naming conventions, throttle threshold omission, test comment error) resolved in 2 iterations. The workflow regressed after a human-requested rebase on Sep 4, when a post-rebase review overrode existing approvals with a CHANGES_REQUESTED verdict containing one out-of-scope finding and one repeated finding. This triggered a fix agent that wasted $1.73 because the PR branch was locked by the merge queue. Cost breakdown: $27.25 total — $21.14 productive (triage through approval + rebase), $6.11 wasted (post-rebase review $4.38 + failed fix $1.73). What went well:
Existing issues corroborated by this PR:
Proposals filed
|
Summary
output_tokens_details.thinking_tokensfrom Claude Code'smessage_deltastream events and accumulate across turnsTokensEvent(progress display) and accumulated total onResultEvent(OTel spans)gen_ai.usage.reasoning_tokenswas always 0 for Claude Code runs but correctly populated for Pi runsChanges
internal/runtime/claude_progress.go:totalReasoning(per-message, reset onmessage_start) andaccReasoning(accumulated) tracking variablesmessage_deltaanonymous struct to parseoutput_tokens_details.thinking_tokensReasoningTokensinTokensEventandResultEventemissionsinternal/runtime/claude_progress_test.go:TestParseClaudeStreamTokensEventWithReasoningTokens— verifies thinking tokens appear in TokensEventTestParseClaudeStreamResultEventAccumulatesReasoningTokens— verifies multi-turn accumulation in ResultEventTestParseClaudeStreamNoThinkingTokensBackwardCompat— verifies 0 when field absent (backward compat)TestProgressParserCapturesReasoningTokensInMetrics— verifies end-to-end flow into RunMetricsTest plan
go test -race ./internal/runtime/...)parseClaudeStream(above 80% threshold)Closes #6904
Post-script verification
agent/6904-claude-reasoning-tokens)32538f078c5cbc1cac5a6d3914d336262f2f8656..HEAD)