fix: include prior tool results in session_history for tool-level evaluators

[liramon2]

Description

TraceExtractor._extract_tool_level builds session_history at trace-level granularity rather than per-tool-span. All tool spans within the same trace receive an identical history snapshot — one that contains only the user prompt and results from previous traces, but none of the tool results from sibling tool calls earlier in the same trace.

This causes ToolSelectionAccuracyEvaluator and ToolParameterAccuracyEvaluator to produce false negatives: the judge model sees a tool call using values that appear "hallucinated" because it cannot see that a prior tool call already returned those values.

Related Issues

Fixes #337

Documentation PR

N/A

Type of Change

Bug fix

Testing

Added test_extract_tool_level_incremental_session_history which verifies that:

• Tool span 0 (square_root) sees only the user prompt in session_history
• Tool span 1 (multiply_numbers) sees the user prompt plus the prior square_root call and its result (42.0)

The test fails without the fix (assert 1 == 2 — second tool only sees user prompt) and passes with it.

• 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]

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

[liramon2]

@strandly-the-agent review this PR. Consolidate findings into a single comment. As for your questions, the limitations are already being tracked as part of: #336

[liramon2]

@strandly-the-agent review this PR. Consolidate findings into a single comment. I am deferring the error-rendering change because it is unrelated to this one.

[liramon2]

@strandly-the-agent review this PR. Consolidate findings into a single comment. For the questions:

1. The scope is deliberate
2. The scope is too big for this PR

[strandly-the-agent]

Round 4 on 2664f4f — ✅ approve. Both scope answers accepted: cross-trace history stays unfiltered by design (not raising it again), and the SpanInfo-boundary normalisation is out of scope — the ADK mapper's tz crash stays a pre-existing follow-up, not yours here.

Verified fixed this round:

• ✅ the tie-broken rule landed — a non-chronological payload through the real OpenInferenceSessionMapper now yields the prior in both orderings (round-3's gap: it silently gave [] before)
• ✅ both mutants that survived at c0407b7 are now caught: target operand start_time→end_time, and _to_aware_utc mapping naive to the wrong offset. The 2.5s end time and the naive tool_long span are doing real work
• ✅ the O(N²) construction is hoisted: N=400 in one trace went 195 ms → 16.5 ms (main: 5 ms)
• ✅ 379 passed (extractors+evaluators) / 732 with mappers+detectors; ruff check, ruff format --check, mypy clean; [BUG] Tool-level evaluators can't see prior tool results within the same trace #337's acceptance prompt still renders verbatim; concurrent / nested-agent / multi-turn matrix unchanged

One new 🟡 below — a one-line clamp I'd land before merge if it's cheap, otherwise worth tracking, because it's the same bug class as #337. The tie-break rule was my suggestion, so I had an independent pass attack it specifically; that's how this surfaced.

Reviewed by strandly-the-agent, an AI agent — a human still approves the merge.

🟡 An inverted span (end_time < start_time) becomes a prior of tools that started earlier

trace_extractor.py:118 trusts end_time, so a span whose end precedes its own start satisfies end <= target_start against everyone — including tools dispatched before it.

Reachable without any malformed evals code: OpenSearchSessionMapper._make_span_info parses both timestamps with _parse_time (opensearch_session_mapper.py:106-107), which returns epoch 1970 for empty/malformed input (:147-155 — pinned by your own tests at test_opensearch_session_mapper.py:418-430). A record with a valid start_time and a truncated/garbled end_time therefore yields start=2026, end=1970. Repro through the real mapper:

  search_flights  start=2026-01-01T00:00:01+00:00 end=2026-01-01T00:00:02+00:00
  book_flight     start=2026-01-01T00:00:03+00:00 end=1970-01-01T00:00:00+00:00  <== INVERTED
  send_email      start=2026-01-01T00:00:05+00:00 end=2026-01-01T00:00:06+00:00

HEAD 2664f4f as shipped:
  target=search_flights  priors=['book_flight']   <-- a result from 2s in its future
  target=book_flight     priors=['search_flights']
  'BOOKED ref=XYZ' in search_flights' judge prompt: True

search_flights and book_flight end up as mutual priors, so two tools in one trace get contradictory histories. main said nothing here, so this is new — and "the judge is told a tool saw a result that didn't exist yet" is exactly what #337's fix is about. (strands_in_memory_session_mapper.py:162 end_time = span.end_time or 0 can produce the same shape; LangChain/OpenInference fall back to now() instead, which errs safe.)

Fix — clamp the end time, verified:

tool_end_times = [
    max(_to_aware_utc(span.span_info.end_time), _to_aware_utc(span.span_info.start_time))
    for span in tool_spans
]

I applied it and re-ran everything: 732 passed, ruff + format + mypy clean, scenario matrix unchanged, the non-chronological case still fixed, and search_flights back to priors=[] with no leak into its prompt.

⚪ The tie-break clause itself is untested (2 surviving mutants) — optional

or position < index is the novelty of this commit, and nothing exercises it, because no fixture uses equal timestamps any more:

drop the tie-break clause entirely      -> 379 passed  (survives)
`or position < index` -> `or True`      -> 379 passed  (survives)

That's the coarse/zero-duration shape a real mapper produces (cloudwatch_session_mapper.py:166-176 sets start_time == end_time == ts for every span). This test kills both (passes at head, ruff-clean, matches the file's style):

def test_extract_tool_level_zero_duration_spans_fall_back_to_span_order():
    """Coarse timestamps (start_time == end_time, e.g. CloudWatch log records) fall back to span order.

    A tool must never appear in its own history, and must never see a later sibling, even when every
    span carries the identical timestamp.
    """
    ts = datetime(2024, 1, 1, 12, 0, 0, tzinfo=timezone.utc)

    def tool_span(span_id, name, result):
        return ToolExecutionSpan(
            span_info=SpanInfo(session_id="test", span_id=span_id, parent_span_id="a0", start_time=ts, end_time=ts),
            tool_call=ToolCall(name=name, arguments={}),
            tool_result=ToolResult(content=result),
        )

    agent_span = AgentInvocationSpan(
        span_info=SpanInfo(session_id="test", span_id="a0", start_time=ts, end_time=ts),
        user_prompt="Weather in Paris, Tokyo and Lima?",
        agent_response="Done",
        available_tools=[ToolConfig(name="get_paris"), ToolConfig(name="get_tokyo"), ToolConfig(name="get_lima")],
    )
    spans = [
        agent_span,
        tool_span("t1", "get_paris", "18C"),
        tool_span("t2", "get_tokyo", "27C"),
        tool_span("t3", "get_lima", "21C"),
    ]
    trace = Trace(spans=spans, trace_id="trace1", session_id="test")
    session = Session(traces=[trace], session_id="test")

    result = TraceExtractor(EvaluationLevel.TOOL_LEVEL).extract(session)

    assert len(result) == 3
    for position, evaluator_input in enumerate(result):
        own_name = evaluator_input.tool_execution_details.tool_call.name
        priors = [e.tool_call.name for m in evaluator_input.session_history if isinstance(m, list) for e in m]
        expected = ["get_paris", "get_tokyo", "get_lima"][:position]
        assert priors == expected, f"{own_name} expected priors {expected}, got {priors}"
        assert own_name not in priors, f"{own_name} appeared in its own history: {priors}"

Side note, since it looks like dead code but isn't: position != index is redundant for well-formed spans (self can only pass the tie-break when end < start) — it's precisely the guard for the inverted-span case above. Keep it.

⚪ Optional polish & follow-ups

• The rule itself is still documented nowhere. types/trace.py:172-174 says "Tool executions that precede it within the same trace" — "precede" is now carrying the whole causality rule. Tightening it to "…from the same trace that completed before it started are appended as a single list[ToolExecution] entry, in span order" would make the invariant (and, per your answer, the deliberate within-a-trace scope) legible without another round.
• Verified sound, for the record: the rule is antisymmetric, transitive, self-excluding and order-independent for all well-formed spans (an exhaustive sweep over timestamp assignments × span orderings found order-dependence confined to exact end == start ties, which is what the tie-break defines). The shared tool_executions list is not mutated after it's appended, extract() is idempotent, and a JSON round-trip is stable.
• Still open by design/agreement, no action asked: prompt volume is uncapped (a 400-tool trace sends ~4.8M tokens of judge input across its calls vs ~16K on main) — a cap or a last-N window would be a reasonable follow-up; the deferred tool_result.error rendering gap; and the pre-existing tz TypeError in adk_otel_session_mapper.py:100,218. Happy to file issues for any of those if useful.

[liramon2]

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

[strandly-the-agent]

Round 5 on baa56ee — ✅ approve-with-nits, mergeable as-is. The clamp is correct and exhaustively verified; the docstring it shipped with isn't — that's the one thing worth fixing, and it's a one-line swap.

Independently verified at this head — five fresh-context passes (adversarial advanced, correctness, test-quality, docs-accuracy, LLM-context); triage skipped API/DevX, correctly, since the delta changes a docstring and no signature/export/type:

• ✅ the clamp (trace_extractor.py:118-121) makes "prior" a strict partial order: 0 invariant violations over the full 4-timestamp grid (65,536 cases) vs 184,320 antisymmetry + 61,440 transitivity violations before it. It's monotone — it can only ever remove a prior, never add one — and it doesn't rewrite span_info, which still carries the raw timestamps
• ✅ reachability of the shape it guards against confirmed by driving real mapper code in 5 of 6 mappers (strands_in_memory:161-162 span.end_time or 0 is the production path); CloudWatch is structurally immune (start_time == end_time always), so there the tie-break carries the weight
• ✅ no regression: every well-formed shape is byte-identical 2664f4f→baa56ee, and all four repo fixtures render identically through their real mappers; 379 tests pass (extractors+evaluators), ruff/mypy clean on both touched files
• 🟡 the new docstring's formula is wrong (types/trace.py:172-174): end_time <= start_time reads as the raw field, but the code compares max(end_time, start_time) against the target's start_time. Executed counterexample below — the one behaviour this commit exists to define is the one the docstring gets wrong
• 🟡 the clamp still has no test: reverting it, neutering it, or defeating the tie-break all leave 379/379 green. Not 🔴 — the shipped behaviour is proven correct on this head, so the exposure is regression protection, not a live defect. Two tests below close it

I'm strandly-the-agent, an AI agent — this is my read, not a merge gate; a human approves.

🟡 Docstring: the executed counterexample, and the fix

Span A has a negative duration (starts 10:05, reports end_time 10:00); target B starts 10:02:

A.end_time (10:00) <= B.start_time (10:02) -> True    <- what the docstring's formula literally says
  actual: target=A  priors=['B']
  actual: target=B  priors=[]                          <- A is NOT a prior of B
=> the code compares max(10:00, 10:05) = 10:05 > 10:02, so A is excluded

So the comparison is against a derived value, not the raw field — and it's the candidate's value against the target's start_time, which the self-referential-looking formula doesn't convey. Replacement (ruff-clean, longest line 94 cols):

    """Input for tool-level evaluators.

    `session_history` never includes the call under evaluation. Within the same trace, a
    tool execution is included only if `max(end_time, start_time)` is at or before this tool's
    `start_time` (the max guards against spans with a negative duration), with list position
    as tiebreaker for equal timestamps. Cross-trace history is unfiltered.
    """

🟡 Two tests that close the gap (verified: pass at head, each kills its mutants)

Both follow the file's existing convention of a local datetime import (as at test_trace_extractor.py:164,248), are ruff check/ruff format clean and ≤120 cols. With both added: 381 passed.

def test_extract_tool_level_clamps_inverted_end_time():
    """A tool with a corrupted (earlier-than-start) end_time is not treated as completing early."""
    from datetime import timedelta, timezone

    base = datetime(2024, 1, 1, 12, 0, 0, tzinfo=timezone.utc)
    agent_span = AgentInvocationSpan(
        span_info=SpanInfo(session_id="test", span_id="a0", start_time=base, end_time=base + timedelta(seconds=20)),
        user_prompt="x",
        agent_response="y",
        available_tools=[ToolConfig(name="broken_tool"), ToolConfig(name="tool_at_5")],
    )
    # starts at t=10 but reports end_time=t=1 (negative duration / corrupted telemetry)
    broken_tool = ToolExecutionSpan(
        span_info=SpanInfo(
            session_id="test",
            span_id="tb",
            parent_span_id="a0",
            start_time=base + timedelta(seconds=10),
            end_time=base + timedelta(seconds=1),
        ),
        tool_call=ToolCall(name="broken_tool", arguments={}),
        tool_result=ToolResult(content="broken_result"),
    )
    tool_at_5 = ToolExecutionSpan(
        span_info=SpanInfo(
            session_id="test",
            span_id="t5",
            parent_span_id="a0",
            start_time=base + timedelta(seconds=5),
            end_time=base + timedelta(seconds=6),
        ),
        tool_call=ToolCall(name="tool_at_5", arguments={}),
        tool_result=ToolResult(content="5_result"),
    )
    trace = Trace(spans=[agent_span, broken_tool, tool_at_5], trace_id="trace1", session_id="test")
    result = TraceExtractor(EvaluationLevel.TOOL_LEVEL).extract(Session(traces=[trace], session_id="test"))

    tool_at_5_input = next(r for r in result if r.tool_execution_details.tool_call.name == "tool_at_5")
    priors = [e.tool_call.name for g in tool_at_5_input.session_history if isinstance(g, list) for e in g]
    assert "broken_tool" not in priors, f"broken_tool (starts t=10) must not be a prior of tool_at_5 (t=5): {priors}"


def test_extract_tool_level_equal_timestamps_break_ties_by_position():
    """Two zero-duration spans sharing a timestamp are ordered by list position, not excluded."""
    from datetime import timedelta, timezone

    base = datetime(2024, 1, 1, 12, 0, 0, tzinfo=timezone.utc)
    agent_span = AgentInvocationSpan(
        span_info=SpanInfo(session_id="test", span_id="a0", start_time=base, end_time=base + timedelta(seconds=5)),
        user_prompt="x",
        agent_response="y",
        available_tools=[ToolConfig(name="tool_first"), ToolConfig(name="tool_second")],
    )
    tool_first = ToolExecutionSpan(
        span_info=SpanInfo(session_id="test", span_id="tf", parent_span_id="a0", start_time=base, end_time=base),
        tool_call=ToolCall(name="tool_first", arguments={}),
        tool_result=ToolResult(content="first_result"),
    )
    tool_second = ToolExecutionSpan(
        span_info=SpanInfo(session_id="test", span_id="ts", parent_span_id="a0", start_time=base, end_time=base),
        tool_call=ToolCall(name="tool_second", arguments={}),
        tool_result=ToolResult(content="second_result"),
    )
    trace = Trace(spans=[agent_span, tool_first, tool_second], trace_id="trace1", session_id="test")
    result = TraceExtractor(EvaluationLevel.TOOL_LEVEL).extract(Session(traces=[trace], session_id="test"))

    priors_by_tool = {
        r.tool_execution_details.tool_call.name: [
            e.tool_call.name for g in r.session_history if isinstance(g, list) for e in g
        ]
        for r in result
    }
    assert priors_by_tool == {"tool_first": [], "tool_second": ["tool_first"]}, (
        f"ties must resolve by list position only, got {priors_by_tool}"
    )

Mutation results (each mutant run against these two tests):

at head                                    2 passed
revert the clamp (back to raw end_time)    1 failed   <- inverted-end-time test
neuter the clamp to max(end, end)          1 failed   <- inverted-end-time test
max -> min                                 1 failed   <- inverted-end-time test
drop the tie-break clause                  1 failed   <- equal-timestamps test
`or position < index` -> `or True`         1 failed   <- equal-timestamps test

The second test has to assert on both tools' prior sets: with the tie-break dropped, tool_second still sees tool_first, and the mutant only shows up as tool_first wrongly seeing tool_second.

⚪ Nit, no action needed

position != index (trace_extractor.py:128) is now unreachable: after clamping, max(end, start) >= start, so a tool can never satisfy the tie-break against itself. Verified by removing it — 379/379 unchanged, and exhaustively identical output across the whole timestamp grid. It was load-bearing before the clamp (an inverted span could list itself), so it's harmless to keep as defensive code — just don't read it as still doing work.