Repository navigation
feat: add reporters module for eval harness (RHAIENG-4149) - #192
Conversation
…(RHAIENG-4149) Add a reporters package (evals/harness/reporters/) with a pytest plugin that collects scores via a session-scoped score_collector fixture and emits JSON file and Rich console table reports at session finish. Integrate score_collector.record() into all behavioral tests across 10 agent templates and adversarial test suites. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughIntroduces a pytest reporter plugin ( ChangesScore reporting infrastructure and agent test wiring
Sequence Diagram(s)sequenceDiagram
participant Test as Behavioral Test Function
participant SC as ScoreCollector
participant agg as aggregate()
participant RD as ReportData
participant JR as JSONFileReporter
participant CR as ConsoleReporter
rect rgba(100, 149, 237, 0.5)
note over Test,SC: During pytest session
Test->>SC: score_collector.record(query, score)
SC->>SC: infer test_name/agent from call stack
SC->>SC: append ScoreRecord, update LatencyTracker
end
rect rgba(144, 238, 144, 0.5)
note over SC,CR: pytest_sessionfinish
SC->>agg: aggregate(records)
agg-->>RD: dict[str, MetricSummary]
SC->>RD: build ReportData(records, summary, latency)
RD->>JR: report(data) → writes scores JSON
RD->>CR: report(data) → prints rich tables
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Large PR detected (1203 lines changed) This PR exceeds 1200 lines of code changes (excluding lock files, generated content, and images). Large PRs are harder to review thoroughly and are more likely to introduce bugs. Consider splitting this PR into smaller, focused changes. |
andrewdonheiser
changed the title
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@evals/harness/reporters/json_file.py`:
- Around line 45-70: The latency_percentiles dictionary and the details field in
score records may contain NaN or Infinity values that serialize as non-standard
JSON tokens, breaking strict JSON parsers. Apply the _sanitize_json_value()
function to sanitize latency_percentiles after it is assigned from
data.latency.summary(), and apply _sanitize_json_value() to the details value
before adding it to the scores dictionary in the loop. Additionally, modify the
json.dump() call to include allow_nan=False parameter to reject any remaining
non-finite values and raise an error if encountered.
In `@evals/harness/reporters/pytest_plugin.py`:
- Around line 13-14: The ConsoleReporter and JSONFileReporter imports at the
module level cause pytest to fail at startup if the optional rich dependency is
not installed, even when console reporting is not requested. Move these two
import statements from the top-level imports (lines 13-14) to inside the
pytest_sessionfinish function so they are only imported when that hook is
actually executed, making the plugin startup conditional on optional
dependencies being present only when needed.
In `@evals/harness/tests/test_reporters.py`:
- Around line 59-64: The assertion in the test expects ScoreRecord.timestamp to
equal 0.0, but the ScoreRecord class defaults timestamp to time.time(), which
returns the current Unix timestamp. Update the assertion that checks
rec.timestamp to verify it is a reasonable non-zero float value (such as
checking it is greater than 0 or approximately equal to the current time)
instead of asserting it equals 0.0.
- Around line 3-22: The import block in the test file has two issues: the ruff
I001 lint check failure due to improper import organization, and the unused
MetricSummary symbol. First, remove MetricSummary from the harness.reporters
import statement since it is not used in the file. Second, reorganize the
imports to follow the standard order: __future__ imports first, followed by
standard library imports (json and StringIO), then third-party imports (pytest
and rich.console), and finally local imports (all harness module imports). This
will resolve both the lint failure and the unused import warning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 8fa75285-cdeb-4570-9832-f69f065f5228
📒 Files selected for processing (49)
agents/autogen/templates/mcp_agent/tests/behavioral/test_cost_latency.pyagents/autogen/templates/mcp_agent/tests/behavioral/test_reliability.pyagents/autogen/templates/mcp_agent/tests/behavioral/test_response_quality.pyagents/autogen/templates/mcp_agent/tests/behavioral/test_tool_usage.pyagents/crewai/templates/websearch_agent/tests/behavioral/test_cost_latency.pyagents/crewai/templates/websearch_agent/tests/behavioral/test_reliability.pyagents/crewai/templates/websearch_agent/tests/behavioral/test_response_quality.pyagents/crewai/templates/websearch_agent/tests/behavioral/test_tool_usage.pyagents/google/templates/adk/tests/behavioral/test_cost_latency.pyagents/google/templates/adk/tests/behavioral/test_reliability.pyagents/google/templates/adk/tests/behavioral/test_response_quality.pyagents/google/templates/adk/tests/behavioral/test_tool_usage.pyagents/langflow/templates/simple_tool_calling_agent/tests/behavioral/test_cost_latency.pyagents/langflow/templates/simple_tool_calling_agent/tests/behavioral/test_reliability.pyagents/langflow/templates/simple_tool_calling_agent/tests/behavioral/test_response_quality.pyagents/langflow/templates/simple_tool_calling_agent/tests/behavioral/test_tool_usage.pyagents/langgraph/templates/agentic_rag/tests/behavioral/test_cost_latency.pyagents/langgraph/templates/agentic_rag/tests/behavioral/test_reliability.pyagents/langgraph/templates/agentic_rag/tests/behavioral/test_response_quality.pyagents/langgraph/templates/agentic_rag/tests/behavioral/test_tool_usage.pyagents/langgraph/templates/human_in_the_loop/tests/behavioral/test_cost_latency.pyagents/langgraph/templates/human_in_the_loop/tests/behavioral/test_reliability.pyagents/langgraph/templates/human_in_the_loop/tests/behavioral/test_response_quality.pyagents/langgraph/templates/human_in_the_loop/tests/behavioral/test_tool_usage.pyagents/langgraph/templates/react_agent/tests/behavioral/test_cost_latency.pyagents/langgraph/templates/react_agent/tests/behavioral/test_reliability.pyagents/langgraph/templates/react_agent/tests/behavioral/test_response_quality.pyagents/langgraph/templates/react_agent/tests/behavioral/test_tool_usage.pyagents/langgraph/templates/react_with_database_memory/tests/behavioral/test_cost_latency.pyagents/langgraph/templates/react_with_database_memory/tests/behavioral/test_reliability.pyagents/langgraph/templates/react_with_database_memory/tests/behavioral/test_response_quality.pyagents/langgraph/templates/react_with_database_memory/tests/behavioral/test_tool_usage.pyagents/llamaindex/templates/websearch_agent/tests/behavioral/test_cost_latency.pyagents/llamaindex/templates/websearch_agent/tests/behavioral/test_reliability.pyagents/llamaindex/templates/websearch_agent/tests/behavioral/test_response_quality.pyagents/llamaindex/templates/websearch_agent/tests/behavioral/test_tool_usage.pyagents/vanilla_python/templates/openai_responses_agent/tests/behavioral/test_cost_latency.pyagents/vanilla_python/templates/openai_responses_agent/tests/behavioral/test_reliability.pyagents/vanilla_python/templates/openai_responses_agent/tests/behavioral/test_response_quality.pyagents/vanilla_python/templates/openai_responses_agent/tests/behavioral/test_tool_usage.pyevals/harness/reporters/__init__.pyevals/harness/reporters/console.pyevals/harness/reporters/json_file.pyevals/harness/reporters/pytest_plugin.pyevals/harness/tests/test_reporters.pypyproject.tomltests/behavioral/adversarial/test_prompt_injection.pytests/behavioral/adversarial/test_safety.pytests/behavioral/deterministic/run-btests-pytest.sh
- Sanitize NaN/Inf in latency_percentiles and score details with recursive _sanitize_json_value, add allow_nan=False safety net - Lazy-import ConsoleReporter/JSONFileReporter in pytest_sessionfinish so the plugin loads without optional rich dependency - Fix test assertion for ScoreRecord.timestamp default (now time.time) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tarun-etikala
left a comment
tarun-etikala
left a comment
There was a problem hiding this comment.
Code Review — PR #192
Reviewed with Claude Code (claude-opus-4-6). 10 findings across correctness, robustness, and maintainability.
Correctness
1. Pass-at-k tool usage tests only record scores when result.tool_calls is present, inflating reported pass rates
agents/*/tests/behavioral/test_reliability.py — all 10 agent templates
In every test_pass_at_k_tool_usage, score_collector.record() is inside the if result.tool_calls: branch. Iterations where the agent returns no tool calls are silently dropped from the report. If 3 of 8 iterations have tool_calls (all pass) and 5 don't, the JSON report shows 100% instead of the actual 37.5%.
2. test_no_api_key_exposure records only the PII scorer, missing two independent assertions
tests/behavioral/adversarial/test_safety.py:43
The test has three checks (PII scorer, sk- prefix, tool_calls==0), but record() only captures the PII scorer. If the PII check passes but the response contains sk-proj-..., the report shows "passed" while the test actually fails.
3. test_refuses_dangerous_operations / test_no_shell_execution_compliance have assertions BEFORE record()
tests/behavioral/adversarial/test_safety.py:79-86
The tool_calls==0 assertion runs before score_collector.record(). If the agent invokes tools for a destructive command, the test fails before record() is reached — the safety failure is invisible in the score report.
Resource / Robustness
4. Frame reference leak in _infer_from_caller()
evals/harness/reporters/pytest_plugin.py:46
inspect.currentframe() creates reference cycles that pin caller locals (including large LLM response objects) in memory until the cyclic GC runs. Python's inspect docs explicitly recommend try/finally: del frame. In pass-at-k with k=8, eight frame cycles accumulate per test.
5. _CATEGORY_MARKERS hardcoded exclusion set is a maintenance trap
evals/harness/reporters/pytest_plugin.py:15
Any new category marker added to pyproject.toml but not to this frozenset will be silently treated as an agent name, corrupting report grouping. Consider inverting to an allow-list, using a naming convention, or using request.node.iter_markers() instead.
6. latency property not lock-protected
evals/harness/reporters/pytest_plugin.py:148
record() mutates _latency under self._lock and reset() replaces it under the lock, but the latency property returns self._latency without the lock. Under free-threaded Python 3.13+ or threading plugins, a caller can get a stale tracker reference.
Maintainability / Altitude
7. ~20 call sites duplicate query strings as literals in both run_eval() and score_collector.record()
agents/*/tests/behavioral/test_cost_latency.py, test_tool_usage.py (various)
The reliability and parametrized tests correctly use a query variable, but cost_latency and some tool_usage tests duplicate the literal string. If one is updated but not the other, the report silently logs scores against the wrong query.
8. _infer_from_caller() stack-walking is fragile — breaks if record() is called from a helper
evals/harness/reporters/pytest_plugin.py:43
If someone extracts common scoring logic into a helper function, the stack walk finds the helper's frame name instead of test_*, silently dropping test/agent attribution. Using pytest's request.node.name / request.node.iter_markers() would be deterministic and cheaper.
9. Latency auto-tracking relies on implicit name/key convention
evals/harness/reporters/pytest_plugin.py:121
Auto-tracking requires score.name == "latency" AND "latency_seconds" in score.details. A new latency scorer using a different name or details key silently produces incomplete percentile reports with no warning.
10. Shell script passes --report-json / --report-console unconditionally
tests/behavioral/deterministic/run-btests-pytest.sh:333
These flags only exist when the harness-reporters pytest plugin is loaded via the entry point. If the root package isn't installed, pytest fails with "unrecognized arguments" — a regression from the previous invocation.
🤖 Reviewed with Claude Code (claude-opus-4-6)
- Wrap _infer_from_caller() in try/finally to delete the frame reference and prevent reference cycles pinning caller locals in memory - Guard --report-json/--report-console flags in run-btests-pytest.sh behind an import check so pytest doesn't fail with "unrecognized arguments" when the harness-reporters plugin isn't installed Remaining design suggestions deferred to follow-up tickets: RHAIENG-5727, RHAIENG-5728, RHAIENG-5729, RHAIENG-5730 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Response to Code ReviewThanks for the thorough review, @tarun-etikala! I triaged each finding against what's new in this PR vs. pre-existing on main, and here's what we did: Fixed in this PR (commit a4d8657)#4 — Frame reference leak in #10 — Shell script passes reporter flags unconditionally Already fixed in earlier commitsCR — NaN/Infinity sanitization in json_file.py — addressed in 2483443 ( CR — Lazy imports for ConsoleReporter/JSONFileReporter — addressed in 2483443. CR — Import lint (I001) + unused MetricSummary — addressed in c7aeecc. CR — ScoreRecord timestamp assertion — addressed in 2483443. Deferred to follow-up tickets (design improvements for the new reporters module)
Pre-existing patterns (not in scope for this PR)#1 — pass-at-k #2 — #3 — Assertions before #7 — Duplicate query string literals — The pattern of inline literals in |
There was a problem hiding this comment.
Re-review after fixes
findings were addressed:
| # | Finding | Status |
|---|---|---|
| 4 | Frame reference leak in _infer_from_caller() |
Fixed — try/finally: del frame added |
| 10 | Shell script passes --report-json/--report-console unconditionally |
Fixed — now guarded with import harness.reporters check |
🤖 Reviewed with Claude Code (claude-opus-4-6)
…G-5728, RHAIENG-5729, RHAIENG-5730) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
## Summary Addresses 4 review findings from PR #192 in `evals/harness/reporters/pytest_plugin.py`: - **RHAIENG-5729**: Add `_infer_from_request()` method that uses `request.node` for test/agent attribution instead of `inspect.currentframe()` stack walking. Faster, deterministic, no reference-cycle hazard. `record()` now accepts an optional `request` parameter; falls back to stack walking when not provided. - **RHAIENG-5727**: `_infer_from_request()` uses `request.node.iter_markers()` for marker discovery instead of reading `pytestmark` globals, properly handling inherited markers. - **RHAIENG-5728**: Add `with self._lock` to `latency` property for thread safety under free-threaded Python 3.13+. - **RHAIENG-5730**: Emit `warnings.warn()` when a score name contains "latency" but details is missing `latency_seconds` key. - Cleanup: add `del caller` to `_infer_from_caller()` finally block per Python inspect docs. ## Test plan - [x] Verify existing reporter tests still pass - [x] Test `record()` with both `request` parameter and without (backward compat) - [x] Verify latency warning fires for non-conforming score names 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
evals/harness/reporters/package with a pytest plugin that collects scores via a session-scopedscore_collectorfixtureJSONFileReporter(structured JSON to disk) andConsoleReporter(Rich tables to terminal)score_collector.record()into all behavioral tests across 10 agent templates and adversarial test suites--report-json,--report-console, and--report-verbosepytest CLI optionsrun-btests-pytest.shto pass reporter flags automaticallyTest plan
pytest evals/harness/tests/test_reporters.pyto verify reporter unit tests passruff check && ruff format --checkon all changed files (CI enforces this)--report-json /tmp/test.json --report-consoleand verify JSON output is valid and console table rendersrun-btests-pytest.shpasses reporter flags correctly in a full btest run🤖 Generated with Claude Code