feat: add Claude Agents OpenInference integration tests - #353
Conversation
|
@strandly-the-agent Review this PR. Only include changes in |
|
@strandly-the-agent Review this PR (integration test only). The plan is to land #340 first and then rebase this on top. |
|
@strandly-the-agent Review this PR (integration test only). |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Round 3 at 62ffc8f — all clear from me. No new findings, nothing blocking.
Both round-2 🔴 are fixed and confirmed live, and the whole module is green and stable: 3 passed on two full-module runs (51.8s, 48.6s, -n auto like CI) plus the multi-agent test 3/3 more times on its own — 5/5 total against live Bedrock.
One of your changes wasn't what I suggested, so I checked it instead of assuming: you deleted expected_assertion rather than repointing it at the answer. That's the better call and I'm withdrawing my concern. It puts both judges in basic mode, which reads the full conversation history rather than the blank-USER QUERY reference path from #355 — so it sidesteps that bug entirely instead of living next to it. I verified the answer is still genuinely graded (mutation evidence below), so there's no coverage hole.
On the red X: Run integration tests still fails on this head, and the evidence says that is not your test. Your module passes locally; tests_integ/test_langchain_openinference_eval.py fails in 1.4s with the LLM call erroring, and the model id it pins is rejected outright in my account — that's #354. Attribution is strong but not proven: I still can't read the CI job logs.
✅ Previous findings — status, verified not assumed
| Finding | Round | Status |
|---|---|---|
🔴 arguments.get("agent_name") always None |
2 | Fixed — now subagent_type. Confirmed live: the delegation assert is load-bearing (it fails on an empty list) and it passes, so delegation genuinely happened on every run. |
🔴 behavioural expected_assertion misgraded by CorrectnessEvaluator |
2 | Fixed by removing it — see the basic-mode analysis below. |
| ⚪ stale Node.js docstring line | 2 | Fixed — line removed. |
⚪ missing tool_result.error assert in test_claude_single_query |
2 | Fixed — mirrored at :138. Passes live, and round-1 mutation testing already proved this assert discriminates. |
🔴 allowed_tools dropped Bash |
1 | Still fixed — 5/5 runs with zero denied tool calls. |
🟡 error handling + 180s timeout, judge-unanimity slicing, tool_result.error, dep bounds, env-var collision |
1 | All still in place and exercised green. |
Why deleting the assertion is fine. _has_assertion / _has_reference both key off expected_assertion, so removing it routes GoalSuccessRateEvaluator and CorrectnessEvaluator to _evaluate_basic. Basic mode formats the judge prompt via _format_trace_level_prompt (evaluator.py:234-254), which includes the full conversation history and the response — unlike reference mode's _extract_user_prompt, which is the one that goes blank (#355).
I mutation-tested that this still grades the answer, using your captured span fixture:
CORRECT ("...difference is 17°F") CorrectnessEvaluator pass=True score=1.0 label='Perfectly Correct'
WRONG(42) ("...difference is 42F") CorrectnessEvaluator pass=False score=0.5 label='Partially Correct'
So a wrong answer still fails the test. expected_output="126" remains unread by these three evaluators, but it now costs nothing — the judges cover the answer and the structural assert covers the delegation, which is the right split.
🔍 Evidence, and what I did not verify
Ran against live Bedrock (us-west-2), claude-agent-sdk 0.2.130 with its bundled CLI:
full module, -n auto: 3 passed in 51.78s
full module, -n auto: 3 passed in 48.59s
multi-agent alone: 1 passed in 40.23s / 41.43s / 43.51s
The #354 attribution:
$ pytest tests_integ/test_langchain_openinference_eval.py
Error in OpenInferenceTracer.on_llm_error callback: TracerException(...)
3 failed in 1.39s # LLM call errored; too fast to be a real invocation
$ converse(us.anthropic.claude-sonnet-4-20250514-v1:0) # pinned at test_langchain_openinference_eval.py:28
ResourceNotFoundException: ... marked by provider as Legacy ...
$ converse(us.anthropic.claude-sonnet-4-6) # what this PR uses
OK
Not verified: whether CI's account behaves the same (the Legacy rejection is account-scoped and I can't read the job logs), so treat the CI-red attribution as strongly-supported rather than proven. My 5 runs are one account, one region, one day — good evidence of stability, not a calibrated flake rate. I also can't rule out that my local langchain failure is partly an artifact of the unbounded langchain>=0.3.0 pin resolving to v1.x (there's a create_react_agent deprecation warning in the output) — which, if so, is the same class of problem as the bound you just added for claude.
Review shape, disclosed: this round was a self-run follow-up pass, not the full specialist fan-out — the 10-line delta only touches findings I filed. I verified each fix against the diff rather than the commit message, re-ran the repros, and attacked the fixes themselves (that's how the round-2 agent_name regression turned up). If you push behaviour changes beyond these fixes, say so and I'll run the full pipeline.
One status note, not a request: this PR still carries #340's mapper commits, so merging it today would land the mapper too — consistent with your plan to land #340 first and rebase, just flagging that the rebase hasn't happened yet.
Automated review. Green here means green in my account on 5 runs — a human should still approve.
|
@strandly-the-agent Review this PR. Consolidate any findings into 1 comment. I rebased over main and removed the Claude mapper changes from this PR (now on main). |
|
@strandly-the-agent Review this PR. Consolidate any findings into 1 comment. The model id is intentionally Sonnet 4 to match the other integration tests. |
|
Update 8/11:
|
|
@strandly-the-agent Review this PR. Consolidate any findings into 1 comment. |
|
@strandly-the-agent Review this PR. Consolidate any findings into 1 comment. |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Round 7 at d26fa4f — all clear. No findings, and I owe you a correction. Single comment as asked.
The unit test closes the last open item, and I confirmed it's a real guard rather than a passenger: mutation-tested it, and it discriminates.
baseline (as shipped) 360 passed in 3.03s
mutant: drop "async_launched" from _TOOL_SUCCESS_STATUSES 1 failed — test_claude_async_launched_tool_status_is_not_an_error
AssertionError at test_...mapper.py:1741
restored 1 passed
That's the whole delta since my last review (359d93b → d26fa4f touches only the unit-test file), so there's nothing else to review.
I was wrong about the model id, and you were right
Run integration tests is SUCCESS on this head — the first green integ run across all seven rounds. Since that job runs hatch test tests_integ over the whole directory, it proves the integ role can invoke us.anthropic.claude-sonnet-4-20250514-v1:0, which is exactly the fact I said I couldn't verify. So my round-4 🔴 was wrong for the environment that actually matters, and "intentionally Sonnet 4 to match the other integration tests" was the correct call. The Legacy rejection I kept hitting is specific to my sandbox account.
Two consequences I'll own rather than leave lying around:
- It also means the langchain integ tests pass in CI, so the premise of #354 (that this id is what's been reddening the job) is refuted for CI's account. I'll update that issue with this evidence rather than leave a misleading bug report open in your tracker.
- Worth knowing why I couldn't confirm it: my token can't read the integ job's logs, so for six rounds "red" was an opaque signal I could only reason around. If you ever want a faster loop from me on integ failures, that permission is the bottleneck.
✅ Full status — everything raised across 7 rounds
| Round | Finding | Status at d26fa4f |
|---|---|---|
| 1 | 🔴 allowed_tools=["Agent"] denied sub-agent Bash (10/12 live failures) |
fixed |
| 1 | 🟡 silent "" on error, no timeout |
fixed (is_error raise + 180s wait_for) |
| 1 | 🟡 judge-unanimity gate | fixed (test_passes[:2], scores[2] >= 0.5) |
| 1 | 🟡 nothing asserted tool_result.error |
fixed in both tests |
| 1 | 🟡 unbounded claude extra |
fixed (>=0.2.0,<0.3.0) |
| 1 | 🟡 ANTHROPIC_MODEL env collision |
fixed (nothing read from env now) |
| 2 | 🔴 arguments.get("agent_name") always None |
fixed (subagent_type) |
| 2 | 🔴 behavioural expected_assertion misgraded by CorrectnessEvaluator |
fixed (removed; basic mode still grades the answer) |
| 2/3 | ⚪ stale Node.js docstring, mirrored error assert | fixed |
| 4 | 🔴 model id → legacy | withdrawn — I was wrong, see above |
| 5 | 🟡 async_launched exemption hollowed out the multi-agent test |
fixed at the root, in the mapper |
| 6 | 🟡 mapper change had no unit test | fixed, and mutation-verified above |
Also green at this head: unit tests on all 10 linux+windows legs (Python 3.10–3.14) and Lint; macOS was still queued. Locally, 360 mapper tests pass, and the integ module was 3/3 green at 359d93b (the integ file is byte-identical here, so I didn't spend another live run re-proving it).
Two standing non-blockers, unchanged and not worth another round: the success set at openinference_session_mapper.py:60 is global across producers (documented tradeoff, I'd leave it), and the PR title/label (feat: / chore) don't quite describe a diff that now includes a src/ fix.
One risk I introduced and still own: assert executed makes the multi-agent test depend on the specialist actually calling Bash. Green in CI and in 5 local runs, but model-dependent — if it ever fails on an otherwise-correct run, that's why.
Review shape: self-run follow-up pass; the delta implemented my own suggestion, so I verified it by mutation rather than re-running the specialist fan-out.
Nothing further from me — this looks ready for a human to approve.
Description
Add integration tests for the Claude Agents SDK mapper.
This also fixes a minor bug caught by the integration tests, where Claude Agents could emit tool spans with status
async_launchedand those spans were wrongfully treated as errors.Related Issues
#329
Documentation PR
Type of Change
New feature
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce new warnings.
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.