Conversation
GitHub Actions
Failure
github-actions
Bot
added
area-evaluators
GitHub Actions
Failure
|
/strands review |
|
@strandly-the-agent Review the changes and provide consolidate feedbacks. Also, I can see that the tool contains certain existing logic in strands-evals. What's your suggestion here? |
|
TL;DR — request changes. The list/get/search shape is the right primitive (MLflow-style progressive disclosure), the diff is tight and additive, and the paranoid read is clean. But four verified defects make the judge score silently wrong — the failure mode this PR exists to eliminate — and #342's actual bug (overflow reported as Verified: head 🔴 Blockers
On the overlap with existing strands-evals logic (your question)Six touchpoints; one is worth fixing now because it's a bug, two deserve a design call, three only look similar — leave them alone. No case for a big shared-helper refactor.
🟡 Should-fix (6)
QuestionsBlocking
Non-blocking Appendix — non-blocking (11)
Also checked, clean: no network/env/subprocess/telemetry anywhere in the diff — I'm an AI reviewer — treat this as prepared input for a human decision, not a gate. Happy to re-review on update. |
pdebjyot
changed the title
GitHub Actions
Waiting
GitHub Actions
Waiting
|
Thanks — this was an unusually useful review; every 🔴 reproduced against the actual code, and the fixes are pushed. Point by point: 🔴 Blockers — all fixed
Overlap — the one that was a bug
🟡 Should-fix
Composition contract / public-API / scope questions
I left the pre-existing #372/#373 as-is (out of scope), and the non-blocking appendix items noted for follow-up. Full suite 1763 passed, ruff clean. Re-review welcome. |
|
@strandly-the-agent review the updated changes. |
|
TL;DR — all four 🔴 blockers are fixed and verified at Status of my previous findings
🟡 New (residuals of the search fix — attack-the-fix findings)
Appendix — non-blocking (5)
Evidence ledger / review shape
I'm an AI reviewer — treat this as prepared input for a human decision. With the two 🟡 addressed (or explicitly deferred), I have nothing further blocking; the open design calls (Q1/Q3, tokens-vs-chars) are |
|
Addendum — the independent adversarial pass finished, and three things are material beyond my comment above (it otherwise agreed: paging survived a 300-session fuzz with zero exactly-once violations,
Smaller items + test gaps the pass found (6)
Repro script + full output are uploaded as review artifacts ( |
GitHub Actions
Waiting
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return "\n".join(parts) | ||
| if isinstance(span, InferenceSpan): | ||
| return "\n".join(_render_message(m) for m in span.messages) | ||
| return str(span.model_dump()) |
There was a problem hiding this comment.
Issue (Important): The search haystack joins fields with \n and preserves the original interior whitespace, but the overview preview collapses all whitespace via _preview (re.sub(r"\s+", " ", ...)). So a phrase a judge copies from a preview misses whenever the source text contained a newline.
Verified on 05c5f2a:
# tool_result.content = "Refund approved.\nAmount: $150 total."
# overview shows: "...approved. Amount..."
search_spans("approved. Amount") # -> "No matches"Copy-from-preview is exactly how a judge forms a query, so this reintroduces the "grounded claim scored as fabricated" failure the search rewrite set out to fix — just for phrases that straddle a line break.
Suggestion: Normalize whitespace on both the haystack and the needle the same way _preview does (re.sub(r"\s+", " ", ...)) before matching. Excerpts are re-previewed anyway, so offset exactness costs nothing.
| return this._window(_span_text(this._spans[index]), offset) | ||
|
|
||
| @tool | ||
| def search_spans(pattern: str, max_matches: int = 20, is_regex: bool = False) -> str: |
There was a problem hiding this comment.
Issue (Suggestion): search_spans("") matches every span with an inflated match count (verified: [0] (42 matches) ...) — an empty needle matches at every character boundary. A confused judge that searches for an empty/whitespace-only string gets meaningless "evidence" rather than a corrective signal.
Suggestion: Return an actionable ERROR: empty pattern (mirroring the existing ERROR:-style messages) when pattern is empty after stripping.
| offset: Character offset for paging through oversized spans. | ||
| """ | ||
| if not 0 <= index < len(this._spans): | ||
| return f"ERROR: index {index} out of range (0..{len(this._spans) - 1})" |
There was a problem hiding this comment.
Issue (Suggestion): On an empty session, get_span(0) returns ERROR: index 0 out of range (0..-1) — the inverted 0..-1 range is model-facing and confusing. (Verified.)
Suggestion: Special-case the empty index with a clearer message, e.g. ERROR: trace has no spans.
| output; ``tools`` is `self.tools`. | ||
| """ | ||
| prompt_section = f"<TraceOverview>\n{self.overview()}\n</TraceOverview>" | ||
| return prompt_section, self.tools |
There was a problem hiding this comment.
Issue (Suggestion): for_judge() is annotated -> tuple[str, list] and returns self.tools by reference. The bare list loses the element type, and handing back the live attribute means a caller mutating the returned list mutates the index's tool set (and two indexes wired to one judge would share/collide).
Suggestion: Parameterize the annotation (tuple[str, list[...]], using the concrete tool type) and return a copy or a tuple so the contract is pinned.
|
Assessment: Comment I re-verified this at head Review themes
The progressive-disclosure design is sound and the responsiveness to prior review has been excellent. |
|
@strandly-the-agent why can't this be a built-in functions when the context / traces are too large? It seems like having these as tool / plugin is not ideal? Investigate the approaches thoroughly. |
|
TL;DR — it can and should be built in. The PR conflates two things: the mechanism (an index the judge LLM queries through Recommendation: keep What is built in today — and where oversized traces currently go (verified)
Approach comparison
Why D isn't ideal, in the maintainer's terms: discoverability (deep import, no export, no in-library consumer — Minimal built-in shape (A + E) — sketch and touch pointsMirror # evaluators/evaluator.py (base) — one helper
def _disclose(self, session: Session, inline: str) -> tuple[str, list]:
"""Inline when it fits; otherwise paged overview + discovery tools."""
if self.disclosure == "never" or not would_exceed_context(self.system_prompt + inline):
return inline, []
section, tools = _TraceIndex(session).for_judge() # internal engine
logger.info("trajectory exceeds judge context; using progressive disclosure")
return section, list(tools)# each judge, at the Agent construction it already has (output_evaluator.py:74, helpfulness_evaluator.py:65, trajectory_evaluator.py:73, multimodal_*)
prompt, disclosure_tools = self._disclose(session, rendered)
agent = Agent(model=self.model, tools=[*(self.tools or []), *disclosure_tools], system_prompt=self.system_prompt, ...)
try:
result = agent(prompt, structured_output_model=EvaluationOutput)
except Exception as e: # reactive path, like failure_detector.py:87-94
if not disclosure_tools and _is_context_exceeded(e):
prompt, disclosure_tools = _TraceIndex(session).for_judge()
result = Agent(..., tools=[*(self.tools or []), *disclosure_tools])(prompt, ...)
else:
raise
What this means for PR #343 and why it wasn't built in
Verified against |
|
Addendum — the independent design pass agrees with the recommendation above, and corrects one detail in my sketch:
|
|
@strandly-the-agent your answer is too tedious: #343 (comment). Summarize it, be neat, simple and clear to understand. What's the recommendation and what will the devx look like. What are the recommended changes? |
|
Fair — here's the short version. Recommendation: make it built in. Evaluators should handle oversized traces themselves; users shouldn't have to know DevX — today (this PR): index = TraceIndex(session)
prompt_section, tools = index.for_judge()
evaluator = OutputEvaluator(rubric=..., tools=tools)
evaluator.evaluate(EvaluationData(input=..., actual_output=f"{answer}\n{prompt_section}"))DevX — proposed: evaluator = TrajectoryEvaluator(rubric=...) # or HelpfulnessEvaluator(), etc. — unchanged
evaluator.evaluate(case) # trace fits → inlined as today
# trace too big → judge gets a paged overview + list/get/search tools, automaticallyNo new imports, no new class to learn. One optional knob if wanted: Recommended changes, in order:
For this PR specifically: either re-scope it to (2)+(3), or merge just the engine under |
|
Agreed — this should be built in, not a plugin the caller wires up. Confirming the plan and re-scoping accordingly. The split: the mechanism (the judge reads a large trace by calling Plan (three steps, in order):
DevX — unchanged for callers: evaluator = TrajectoryEvaluator(rubric=...) # or HelpfulnessEvaluator(), etc.
evaluator.evaluate(case) # trace fits → inlined as today
# trace too big → judge auto-gets overview + list/get/search toolsOne optional knob if you want it: One correction to my earlier sketch: For this PR: (1) is already up as #399. I'll re-scope this PR to (2)+(3) — stage the engine as internal and add the base-class wiring. Sound right before I push the changes? |
|
@strandly-the-agent review the changes based on the discussions and proposed structure. |
|
TL;DR — there's nothing new to review yet. The PR head is still From my side: yes, the plan is right — it matches what was agreed (engine internal, wiring in the evaluator base, #399 first). One heads-up: a What the re-scoped push should contain (review checklist)
I'm an AI reviewer — this is prepared input for a human decision. |
… agents
Large agent trajectories overflow a judge model's context window when inlined
into the evaluation prompt, forcing Experiment error-isolation to record the
case as score:0 / test_pass:False — a false failure for a correct agent.
TraceIndex builds an in-memory index over a Session and exposes:
- overview(): one compact line per span (index, type, tool, sizes, preview)
that always fits the judge context, substituted for the full trajectory.
- three discovery tools the judge calls on demand: list_spans / get_span /
search_spans (mirrors MLflow's ListSpans/GetSpan/SearchTraceRegex).
get_span pages oversized spans via max_read_chars + offset so no single tool
return can itself overflow the judge. Backend-agnostic: consumes any Session
produced by a provider/mapper.
… TraceIndex Covers overview() formatting, list_spans/get_span/search_spans behavior, offset paging on oversized spans, and end-to-end use through OutputEvaluator with tools=index.tools.
Compares OutputEvaluator judging grounded vs fabricated claims two ways — full trajectory inlined vs overview + discovery tools — asserting the index-equipped judge separates grounded from fabricated where inline overflows. Skips without live Bedrock credentials.
…, expose system_prompt/tools, tz-safe sort, add for_judge()
…gex anchors Addresses the adversarial review's three residual yellows on TraceIndex, plus the pre-existing mypy failures on this branch (CI runs ruff + mypy; only ruff was green before). - search_spans now caps its accumulated output at max_read_chars (not max_matches alone), so a generous max_matches on a long trace can no longer return 13x the per-call budget. Emits a 'budget reached' marker distinct from the max_matches marker. - Inference-span search rendered messages via model_dump(), leaking 'error': None into the haystack (false positive on every inference-span 'error' search) while a genuinely failed tool inside an inference span was unfindable. Messages are now rendered from their fields (role + text + tool name/args/result), and tool errors carry a searchable 'error:' token in both the tool and inference branches, matching the overview's [ERROR] vocabulary. - Regex search compiles with re.MULTILINE so ^/$ anchor to line boundaries in the newline-joined haystack instead of silently never matching. - mypy: annotate hits/lines empty-list locals and rename the regex exception variable so it no longer collides with the match-position unpacking. README and search_spans docstring updated to state search is bounded too.
…ch, empty-pattern/empty-session guards, for_judge copy)
…al TraceIndex
Wire progressive trace disclosure into every judge evaluator behind a new
`disclosure` kwarg ("auto" | "always" | "never", default "auto"). When a
Session trajectory would overflow the judge's context window, the judge now
receives a compact <TraceOverview> plus the list_spans / get_span /
search_spans tools and reads the trace on demand, instead of silently
mis-scoring a correct agent on a truncated/overflowed prompt.
- Relocate the disclosure engine from the public tools/ surface to the
internal evaluators/_trace_index module; it is an evaluator implementation
detail, not user-facing API.
- Add the disclosure seam to the base Evaluator (_render_with_disclosure,
_resolve_disclosure_index, _disclosed_trace_section, disclosure-aware
_format_* helpers, _tool_level_render, _validate_disclosure) and wire all
18 evaluators through it, including the bespoke composers
(compose_test_prompt trajectory_override; per-skill/per-tool resolution
shared once per case).
- "auto" discloses only on preflight overflow; "always" whenever a Session is
present; "never" always inlines (real overflow -> could-not-evaluate).
A fitting case stays byte-identical to before with no extra tools.
- Add unit + end-to-end disclosure tests; update README to the automatic story.
Adds a public `disclosure` kwarg -> needs-api-review.
- Type disclosure kwarg as DisclosureMode (Literal) across all 18 evaluators instead of bare str, so misuse is caught at type-check time. - Resolve TraceIndex once per case in tool-level and skill evaluators rather than re-probing inside the per-tool / per-skill loop. - Skill probe uses the full inline prompt (body + response), not just the trajectory, so the auto threshold reflects real prompt size. - Skip the inline-render probe for always/never modes (probe is irrelevant). - Fix README + docstrings: genuine overflow under "never" surfaces as a judge context-length error (the could-not-evaluate path is strands-agents#399, not this branch).
05c5f2a to
a8e0a87
Compare
GitHub Actions
Waiting
GitHub Actions
Waiting
4 participants
feat(tools): TraceIndex — progressive trace disclosure for judge-based evaluators
Branch:
feat/trace-indexBuilds on: #324 (custom
tools=onOutputEvaluator/TrajectoryEvaluator)Addresses: #342 (item (2); items (1) and the trajectory path remain open)
Problem
When the trajectory handed to a judge-based evaluator is larger than the judge model's context window, the judge call raises
ContextWindowOverflowException.Experiment._run_evaluatorcatches it under error isolation and records the case asscore: 0, test_pass: False— indistinguishable from a genuine quality failure. A correct agent response gets a false-negative failing score purely because its trace was too big for the judge to read.This isn't a theoretical edge. We observe it on real production agent traces, where routine multi-step sessions serialize past a 200K-token judge window and the largest run into the millions of tokens — so under a default Sonnet-class judge, correct agents are being silently scored as failures today. The problem is stack-agnostic: we reproduced identical overflow behavior across strands-evals, DeepEval, and a Langfuse-style managed judge on a shared Bedrock judge model (three distinct error signatures, same trace-size cliff).
The measured evidence below is from a synthetic, deterministic, offline benchmark — no real data — so it's independently reproducible.
What this PR adds
TraceIndex— an in-memory index over aSessionthat lets a judge read a large trace without inlining it. It is an established pattern: MLflow's Agent-as-a-Judge trace scorers hand the judgeListSpans/GetSpan/SearchTraceRegex, and Zhuge et al.'s Agent-as-a-Judge (arXiv:2410.10934) usesretrieve/read/locatemodules to pull only the relevant segments.overview()— one compact line per span (index, type, tool name, sizes, preview, tool ok/error status). Paged by span index throughmax_read_chars, so even the first documented call on a huge trace can't overflow. The header tells the judge previews are truncated and that claims must be verified with the tools before scoring.list_spans/get_span/search_spans— discovery tools the judge calls on demand (mirrors the MLflow triad).get_spanpages oversized spans viamax_read_chars(default 8000) +offset;search_spansis literal by default (opt-inis_regex=True) and searches an unescaped rendering of every visible field, includingsystem_promptandavailable_tools, so a literal a judge copies from the overview ($150) matches.Backend-agnostic — consumes any
Sessiona provider/mapper produces; timestamps are tz-normalized before sorting so a mix of naive/aware spans (from different mappers) can't crash construction.for_judge()— returns(prompt_section, tools)together, so the two composition halves (overview into the judged output, tools onto the evaluator) can't be done half-way and silently degrade the judge.Two disclosure strategies compose from these pieces: index (substitute
overview()into the prompt — portable, works with any judge) and explore (index + discovery tools via #324'stools=— Strands-native, best accuracy).Evidence
Cross-framework matrix (1344 cells). 200-trace labeled corpus × 3 frameworks (strands-evals, DeepEval, Langfuse-style) × 4 metrics (groundedness, accuracy, trajectory, tool_use) × inline/index/explore, all bound to one shared Bedrock judge. Each metric scored as a binary classifier against planted ground truth (overflow ⇒ wrong prediction). These numbers are external motivation from the offline benchmark above — they are not reproducible from anything in-tree:
indexandexplorehold accuracy on traces that fit and recover it on traces that overflow (inline is unusable in the overflow bucket; index/explore score correctly).exploretools beat a bare index when the decisive fact is buried in a large tool result: onwrong_tooltraces the deciding refund amount sits inside a large search result thatoverview()elides, so the index judge false-fails groundedness (0.83);get_span/search_spanslet the judge retrieve it → groundedness 1.00.Ground-truth A/B (grounded / fabricated claims, evidence buried mid-trace): the index-equipped judge separates grounded from fabricated where the inline judge overflows — captured as the integ test
test_judge_reliability_inline_vs_explore.Known limitation (called out honestly)
TrajectoryEvaluatorinlinesactual_trajectoryunconditionally (case_prompt_template.py:51), so the trajectory metric still overflows on large traces even with the index — the substitution only reaches evaluators that route through the caller-controlledactual_output(the output-family metrics, fixed today). Making the trajectory metric disclosure-aware needs a template change and is proposed as a follow-up; this PR does not change that path.Separately, this PR adds the capability but does not change
_run_evaluator's error handling — distinguishing harness overflow from a qualityscore: 0is tracked in #342 as an independent change.Testing
system_prompt/available_toolsexposure; tool-error surfacing;max_read_chars/offset validation; mixed naive/aware timestamps;for_judge()composition; cross-pattern; end-to-end throughOutputEvaluatorwithtools=index.tools) — all pass.ruff check/ruff formatclean.Revision (in response to review)
overview()/list_spansnow page by span index throughmax_read_chars, so the judge's first call can't overflow.search_spansis literal by default (opt-inis_regex), searches an unescaped rendering, counts per-span matches, signals truncation, and no longer false-positives on serialization artifacts like"error": null.get_span/search_spansnow exposesystem_promptandavailable_tools.OutputEvaluatorwith an explicitTrajectoryEvaluatorlimitation note._flatten_spanssorts via the repo's_to_aware_utc, fixing the mixed naive/awareTypeError(root cause [BUG] SessionMapper.parse_timestamp returns naive datetimes despite docstring promising timezone-aware UTC — mixed sessions break datetime comparisons #372).max_read_chars/negative-offset validation, tool ok/ERROR status + error preview and INFERENCE size/preview in the overview, truncation markers onsearch_spans, and a verify-before-score header the model actually sees.for_judge()→(prompt_section, tools)so the overview and tools are wired together atomically instead of through two independent, silently-degrading steps.Checklist