Skip to content

fix: select root agent span by earliest start_time in multi-agent traces - #371

Draft
liramon2 wants to merge 1 commit into
strands-agents:mainfrom
liramon2:root-agent-fix
Draft

fix: select root agent span by earliest start_time in multi-agent traces#371
liramon2 wants to merge 1 commit into
strands-agents:mainfrom
liramon2:root-agent-fix

Conversation

@liramon2

@liramon2 liramon2 commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes root-agent selection in multi-agent traces so _find_root_agent_span breaks ties by earliest start_time. In a flat multi-agent trace where multiple agent spans are parentless, the previous logic returned the first parentless-with-content span in list order, which may be an arbitrary sub-agent rather than the outer coordinator. Since the coordinator is invoked first and encloses its sub-agents, selecting the earliest-start agent picks the root agent correctly.

Related Issues

Fixes #370

Additionally, this is a prerequisite for #365 and #366. These PRs convert OpenAI Agents SDK traces that may contain multiple parentless span. When an OpenAI agent hands off or delegates to another agent, the original agent's span is a sibling rather than parent of the subsequent agent's span; therefore, there are multiple parentless agent spans. That leads the scenario described in this PR's issue.

Documentation PR

N/A

Type of Change

Bug fix

Testing

  • I ran hatch run prepare

Checklist

  • I have read the CONTRIBUTING document
  • I have reviewed and understand every line of code in this PR, including any generated by AI tools, and I can explain why it works
  • My change is focused and reasonably small; I have split unrelated work into separate PRs
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@liramon2
liramon2 requested a review from a team as a code owner August 15, 2026 02:16
@liramon2
liramon2 requested a review from Unshure August 15, 2026 02:16
@liramon2
liramon2 marked this pull request as draft August 15, 2026 02:16
@liramon2
liramon2 removed the request for review from Unshure August 15, 2026 02:16
@github-actions github-actions Bot added area-tracing Trace/session ingestion: providers, session mappers, extractors, telemetry/OTEL bug Something isn't working labels Aug 15, 2026
@liramon2

Copy link
Copy Markdown
Contributor Author

@strandly-the-agent Review this PR. Consolidate any findings into a single comment.

@strandly-the-agent

This comment was marked as resolved.

@liramon2

liramon2 commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

@strandly-the-agent Review this PR. Consolidate any findings into a single comment.

The durable fix is at the boundary — make parse_timestamp honour its own docstring ("Returns: Timezone-aware datetime in UTC") by normalising naive values instead of returning them unchanged.

This is effectively a no-op for the _find_root_agent_span because the timestamp is normalized in _to_aware_utc. The conversion may happen earlier, but the operation is the same.

One line-number correction worth having, since it changes where a fix would go: the wrong list comes from the per-agent scoping branch at :138-140, not the available_tools fallback at :112-113/:142. With agent_span_id set and len(agent_spans) > 1 the fallback is never reached, so a union built at :112-113 alone would not fix this. Decoupling the tool list from root selection means touching :138-140 — e.g. union the agent spans' tool lists when the owner came from the root fallback (a guess) rather than from the parent-chain walk — or not assigning a guessed owner to genuinely unattributable tool spans at all.

This is outside the scope of the PR, which is targeting the root agent search.

Question 1: a coordinator without content would most likely error on evaluations, so it is out of scope here.
Question 2: deferring to minimize changes to this PR.

@strandly-the-agent

Copy link
Copy Markdown

TL;DR — approve. All five findings from my previous review are resolved on 9fa6675, and I accept both of your rulings (one with a footnote below). No new findings — the src/ delta is docstring-only, everything else is tests, so this was a fixes-only follow-up review rather than a full fresh-pass fan-out.

Previous findings — status (each verified against the diff, not the commit message):

finding status
🟡 1 root selection depends on timestamp quality ✅ in-PR asks delivered: naive/aware-mix test added (kills the _to_aware_utc mutant), docstring documents the tiers. Your no-op ruling: agreed for root selection — footnote below.
🟡 2 available_tools follows the root ✅ closed as out of scope per your ruling — and the new extractor test's third assertion (['ask_research', 'ask_math'] for the orphan get_weather) pins the current shape, which was exactly the fallback I asked for.
🟡 3 no test through the reported path test_tool_level_flat_multi_agent_anchors_to_earliest_coordinator added — I ran it against origin/main's code and it fails there, so it genuinely pins the regression.
🟡 4 docstring under-describes ✅ tier ordering restored.
🟡 5 three surviving mutants ✅ all killed — I re-applied each mutation (drop or parentless, drop or s.agent_response, drop _to_aware_utc) and each now fails exactly one test.
❓ blocking question (contentless coordinator) settled — the restored docstring documents the ordering, which was one of the two ways I said would close it. The shape remains by design; noted, not re-raised.

Verified on 9fa6675:pytest tests/strands_evals/{types,extractors,mappers}499 passed locally · ✅ mutation re-runs as above · ✅ differential vs main for the new tests · ✅ CI Lint green (closes my earlier "ruff/mypy unverified" caveat) and the full unit-test matrix green · ⚠️ one CI note below.

⚠️ Run integration tests failed on this head but passed on 72efc62. The delta between the two is a docstring in src/ plus unit tests, which can't plausibly change live-model behaviour, and I can't read the job logs (no access from my side) — so it looks like judge/infra flakiness rather than this PR. Worth a re-run before merge to confirm: https://github.com/strands-agents/evals/actions/runs/31890316639/job/95025516391

Footnote on ruling 1 (parse_timestamp normalisation)

Agreed on the naive-datetime half: normalising naive → UTC in parse_timestamp is the same replace(tzinfo=UTC) operation _to_aware_utc already performs at the comparison site, so for _find_root_agent_span it's a no-op — the true offset of a naive value is unrecoverable either way. Conceded.

The half that argument doesn't cover is the fabricated datetime.now(UTC) for None/unparseable input (session_mapper.py:93-94, :104-105): no downstream guard can undo a fabricated timestamp, and that's what drove the missing/malformed-timestamp repro in my first review. That behaviour is pre-existing and out of this PR either way — mentioning it only as a candidate for a separate issue; happy to file it with the repro if you want, or drop it if you consider degraded exports out of contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-tracing Trace/session ingestion: providers, session mappers, extractors, telemetry/OTEL bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

_find_root_agent_span selects an arbitrary sub-agent as root in flat multi-agent traces

2 participants