DEV-1616: roll the v1 discovery sub-agent's turns/tokens/cost into the task total - #71
Merged
ZmeiGorynych merged 2 commits intoJul 1, 2026
Conversation
…e task total The two-stage claude_sdk_*_v1 agents delegate discovery to a warm second ClaudeSDKClient reached via ask_discovery. Discovery tokens/cost/n_calls already flowed into the shared accum (each ask builds a fresh SdkUsageTracker with the default scope="agent"), but the headline n_agent_turns was backfilled from the MAIN client's trajectory only — so discovery turns were off-book — AND that backfill counted block-level AssistantMessage events, not dedup'd turns. Fix: - SdkUsageTracker.committed_n_calls: the exact turn count a tracker commits to its agent::<model> breakdown row (max(1, turns) with a ResultMessage). - DiscoveryChannel.turns: sums each ask's committed count across all paths (success / timeout / error), so partial asks still contribute. - discovery_runtime.DiscoveryRollup + run_main_with_discovery fills it in the finally (crash-safe) from the channel; docstring corrected. - finalize_result_row: n_agent_turns is now the scope=="agent" breakdown sum (dedup'd turns, inclusive of discovery) for claude_sdk-shaped rows — gated so agno/smolagents (which also write scope="agent" usage but emit no AssistantMessage entries) keep their trajectory count. n_discovery_turns defaults to 0. - The 4 v1 agents create/pass DiscoveryRollup and surface usage["n_discovery_turns"] on both success and exception paths. n_discovery_turns rides in per-row usage_json (like n_ask_user_calls); it is not a TokenUsage field, so run-level total_usage aggregation does not sum it (per-task is the contract). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DEV-1616 Roll up the v1 discovery sub-agent's turns / tokens / cost into the task total usage
ProblemThe two-stage v1 agents ( Evidence
Why it matters
FixEnsure the discovery client's usage is fully rolled into the task total:
Scope
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds committed-call tracking, cumulative discovery-turn accounting, rollup propagation into agent usage, harness backfills for turn counts, and test coverage for success and crash paths. ChangesDiscovery Turn/Usage Rollup
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
tests/test_dev1616_discovery_usage_rollup.py (3)
246-250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: unused unpacked variable
trajectory.Ruff (RUF059) flags
trajectoryas unpacked but never used in this test.♻️ Proposed fix
- accum, trajectory, rollup, disc = await _drive( + accum, _trajectory, rollup, disc = await _drive(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_dev1616_discovery_usage_rollup.py` around lines 246 - 250, The test unpacks trajectory in test_rollup_includes_discovery_turns_tokens_and_calls but never uses it, triggering Ruff RUF059. Remove the unused unpacked variable from the _drive(...) assignment or replace it with an ignored placeholder, keeping the rest of the assertions and the accum/rollup/disc symbols unchanged.Source: Linters/SAST tools
105-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: prefer
next()over single-element list slice.Static analysis (RUF015) flags this pattern.
♻️ Proposed fix
- assert [r for r in accum.breakdown if r.scope == "agent"][0].n_calls == 1 + assert next(r for r in accum.breakdown if r.scope == "agent").n_calls == 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_dev1616_discovery_usage_rollup.py` around lines 105 - 113, The test in SdkUsageTracker should avoid building a list just to grab the first matching breakdown entry, since static analysis flags that as inefficient. Update the assertion in test_committed_n_calls_result_only_is_one to use next() over the TokenUsage.breakdown iterator with the same scope filter, while still verifying the agent n_calls value remains 1.Source: Linters/SAST tools
306-345: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: prefer
next()over single-element list slice.Same RUF015 pattern as the earlier test.
♻️ Proposed fix
- assert [r for r in accum.breakdown if r.scope == "agent"][0].prompt_tokens == 80 + assert next(r for r in accum.breakdown if r.scope == "agent").prompt_tokens == 80🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_dev1616_discovery_usage_rollup.py` around lines 306 - 345, The test in `test_crash_path_still_reports_discovery_turns` uses a single-element list comprehension and index access to get the first agent breakdown entry; replace that pattern with `next()` to match the existing RUF015 style used elsewhere. Keep the assertion against `accum.breakdown` and `scope == "agent"` intact, but fetch the matching item via `next(...)` so the test is consistent and avoids the single-element slice pattern.Source: Linters/SAST tools
tests/test_claude_sdk_otf_v1_agent.py (1)
664-737: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM! Correctly validates that
DiscoveryRollupvalues survive both the success and crash-after-discovery paths, withn_agent_turnsreflecting main+discovery.Optional: mirror this rollup-wiring coverage on the other three v1 agent flavors.
Only
claude_sdk_otf_v1gets this deeper success/exception rollup-surfacing test;claude_sdk_otf_raw_v1,claude_sdk_otf_ainteract_v1, andclaude_sdk_otf_ainteract_raw_v1only have the turn-counting test. Since all four wireDiscoveryRollupthrough nearly identicalrun_taskshapes, the same_fake_rmwd_factorypattern could be reused for parity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_claude_sdk_otf_v1_agent.py` around lines 664 - 737, Add the same DiscoveryRollup surfacing coverage to the other v1 agent variants for parity. Reuse the existing _fake_rmwd_factory pattern and extend the run_task tests in claude_sdk_otf_raw_v1, claude_sdk_otf_ainteract_v1, and claude_sdk_otf_ainteract_raw_v1 so they verify n_discovery_turns and n_agent_turns on both the success path and the crash-after-discovery path, matching the behavior already asserted in ClaudeSDKOtfAgent.tests/test_claude_sdk_otf_ainteract_raw_v1_agent.py (1)
628-664: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: dedupe
_shared_turn_msgs()across the four v1 agent test files.This helper is copy-pasted identically into
test_claude_sdk_otf_ainteract_v1_agent.py,test_claude_sdk_otf_raw_v1_agent.py, andtest_claude_sdk_otf_v1_agent.py. Consider hoisting it into a shared test fixture/module to avoid drift across the four flavors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_claude_sdk_otf_ainteract_raw_v1_agent.py` around lines 628 - 664, The `_shared_turn_msgs()` helper is duplicated across the v1 agent test files, so consolidate it into a shared test fixture or utility module and have each test file import/use that single helper instead of maintaining four copies. Keep the existing behavior of the `AssistantMessage`/usage stub intact, and update the tests that reference `_shared_turn_msgs()` to use the shared definition so the agent test variants stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_claude_sdk_otf_ainteract_raw_v1_agent.py`:
- Around line 628-664: The `_shared_turn_msgs()` helper is duplicated across the
v1 agent test files, so consolidate it into a shared test fixture or utility
module and have each test file import/use that single helper instead of
maintaining four copies. Keep the existing behavior of the
`AssistantMessage`/usage stub intact, and update the tests that reference
`_shared_turn_msgs()` to use the shared definition so the agent test variants
stay in sync.
In `@tests/test_claude_sdk_otf_v1_agent.py`:
- Around line 664-737: Add the same DiscoveryRollup surfacing coverage to the
other v1 agent variants for parity. Reuse the existing _fake_rmwd_factory
pattern and extend the run_task tests in claude_sdk_otf_raw_v1,
claude_sdk_otf_ainteract_v1, and claude_sdk_otf_ainteract_raw_v1 so they verify
n_discovery_turns and n_agent_turns on both the success path and the
crash-after-discovery path, matching the behavior already asserted in
ClaudeSDKOtfAgent.
In `@tests/test_dev1616_discovery_usage_rollup.py`:
- Around line 246-250: The test unpacks trajectory in
test_rollup_includes_discovery_turns_tokens_and_calls but never uses it,
triggering Ruff RUF059. Remove the unused unpacked variable from the _drive(...)
assignment or replace it with an ignored placeholder, keeping the rest of the
assertions and the accum/rollup/disc symbols unchanged.
- Around line 105-113: The test in SdkUsageTracker should avoid building a list
just to grab the first matching breakdown entry, since static analysis flags
that as inefficient. Update the assertion in
test_committed_n_calls_result_only_is_one to use next() over the
TokenUsage.breakdown iterator with the same scope filter, while still verifying
the agent n_calls value remains 1.
- Around line 306-345: The test in
`test_crash_path_still_reports_discovery_turns` uses a single-element list
comprehension and index access to get the first agent breakdown entry; replace
that pattern with `next()` to match the existing RUF015 style used elsewhere.
Keep the assertion against `accum.breakdown` and `scope == "agent"` intact, but
fetch the matching item via `next(...)` so the test is consistent and avoids the
single-element slice pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 16e9b6d6-5dce-4fe6-83c1-afb835e40123
📒 Files selected for processing (15)
src/bird_interact_agents/agents/claude_sdk/agent.pysrc/bird_interact_agents/agents/claude_sdk/discovery_channel.pysrc/bird_interact_agents/agents/claude_sdk/discovery_runtime.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract_raw_v1/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract_v1/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_raw_v1/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_v1/agent.pysrc/bird_interact_agents/harness.pytests/test_claude_sdk_otf_ainteract_raw_v1_agent.pytests/test_claude_sdk_otf_ainteract_v1_agent.pytests/test_claude_sdk_otf_raw_v1_agent.pytests/test_claude_sdk_otf_v1_agent.pytests/test_dev1581_discovery_channel.pytests/test_dev1616_discovery_usage_rollup.pytests/test_finalize_result_row.py
…eview) Codex review: a crash after some ask_discovery calls but before any MAIN AssistantMessage was recorded leaves the main trajectory empty, so the traj_turns>0 gate skipped the breakdown derivation and n_agent_turns stayed 0 — despite discovery turns being in the agent-scope breakdown and a positive usage.n_discovery_turns. Also derive from the breakdown when n_discovery_turns > 0 (a claude_sdk-only signal), keeping the headline inclusive of discovery. Regression test added. CodeRabbit nitpick: rename the unused unpacked `trajectory` to `_trajectory` in the rollup test (RUF059). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Problem
The two-stage
claude_sdk_*_v1agents delegate schema/entity discovery to a warm secondClaudeSDKClientreached via the in-processask_discoverytool (discovery_runtime.run_main_with_discovery+discovery_channel.DiscoveryChannel). The headlinen_agent_turnsundercounted the real cost: it was backfilled from the MAIN client's trajectory only, so discovery turns were off-book — and that backfill counted block-levelAssistantMessageevents (the live SDK emits one per content block), not dedup'd turns.Tokens / cost /
n_callswere verified to already include discovery: each ask builds a freshSdkUsageTracker(accum, model)with the defaultscope="agent", so its usage lands in the sameagent::<model>breakdown row and inagent_cost_usd.Fix
SdkUsageTracker.committed_n_calls— the exactn_callsa tracker commits to its breakdown row (max(1, turns)with a terminalResultMessage).DiscoveryChannel.turns— sums each ask's committed turn count across every path (success / timeout / error), so partial asks still contribute.DiscoveryRollup+run_main_with_discoveryfills it in thefinally(crash-safe) from the channel; docstring corrected.finalize_result_row—n_agent_turnsis now thescope=="agent"breakdown sum (dedup'd turns, inclusive of discovery) for claude_sdk-shaped rows. Gated so agno/smolagents (which also writescope="agent"usage but emit noAssistantMessagetrajectory entries) keep their trajectory-derived count.n_discovery_turnsdefaults to0.DiscoveryRollupand surfaceusage["n_discovery_turns"]on both success and exception paths.This makes v0/v1/v2 turn counts apples-to-apples (the block over-count is fixed for every claude_sdk agent, not just v1).
Scope note
n_discovery_turnsrides in per-rowusage_json(liken_ask_user_calls); it is not aTokenUsagefield, so run-leveltotal_usageaggregation does not sum it — per-task is the contract.Tests
New
tests/test_dev1616_discovery_usage_rollup.py(trackercommitted_n_calls; a real-trackerrun_main_with_discoveryintegration covering the turn/token/cost rollup, per-ask-not-cumulative parity, and the crash path) plus additions to the channel, finalize (incl. the agno/smolagents non-derivation regression), and all four v1-agent test files. Full non-integration suite: 3825 passed, 94 skipped.Plan and tests were adversarially reviewed by Codex in two rounds; all findings folded in.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
0when missing, andn_agent_turnsis derived correctly even when trajectories are empty.Tests