fix(pipeline): log retrieval failures so a broken retriever is distinguishable from an empty one - #262
Merged
himanshu231204 merged 5 commits intoJul 31, 2026
Conversation
…HQ#256) A retriever whose retrieve() raises currently degrades to the dataset-context fallback with nothing logged and nothing in the run's errors, making a broken retriever indistinguishable from a clean empty retrieval. This test asserts the fallback still applies while the failure is recorded in errors and logged. Fails on unfixed code. Co-Authored-By: Kimi K3 <noreply@kimi.com>
Pipeline._retrieve swallowed every exception in a bare except and silently degraded to the dataset-context fallback, making a broken retriever indistinguishable from a clean empty retrieval. The fallback behaviour is unchanged; the failure is now logged naming the exception and recorded in the run's errors in the same shape used by the per-item failure path. Also log when inspect.signature fails and the retriever is assumed to lack ground_truth_contexts support. Co-Authored-By: Kimi K3 <noreply@kimi.com>
Recording the non-fatal retrieval degradation in result.errors broke engine.py's summary arithmetic: successful_evaluations is computed as len(results) - len(errors), and the retrieval-failure entry has no paired placeholder result, so a healthy run was mis-counted as failed. That polluted the pipeline summary, every report renderer's failure section, the CLI error count, and the cicd plugin gate metrics, failing CI for users gating on failed_evaluations == 0. Remove the result.errors append; the failure is still logged naming the exception and the dataset-context fallback is unchanged. The regression test now pins the log-only contract: it asserts result.errors is empty and the run still counts as successful, which catches any future change that reintroduces the errors entry. Co-Authored-By: Kimi K3 <noreply@kimi.com>
Leftover from the log-only rework: the item and result parameters were added only to support the result.errors append, which was removed when the fix became log-only. Nothing in the body uses either parameter, so revert _retrieve to its original (question, context, gt_contexts) signature and the call site to the original three arguments. Logging, fallback behaviour, and the docstring rationale are unchanged. Co-Authored-By: Kimi K3 <noreply@kimi.com>
Member
|
/oc review pr |
|
🎉 Congratulations @Nitjsefnie! Your pull request has been successfully merged into main. 🚀 Thank you for contributing to OpenAgentHQ and helping improve the project. We truly appreciate your contribution and hope to see you back with more amazing PRs! Happy Open Sourcing! ❤️ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Pipeline._retrievewrapped retrieval in a bareexcept Exceptionand degraded to the dataset-context fallback with nothing logged, so a completely broken retriever — bad credentials, unreachable vector store, a bug in its own code — produced exactly the same observable result as a retriever that ran fine and found nothing.The fallback is unchanged. The failure is now logged, naming the exception, so a degraded run can be told apart from a clean one. The
inspect.signaturefallback next to it gets the same treatment.Type of Change
Related Issues
Closes #256
How Has This Been Tested?
uv run pytest tests/unit -v --tb=short→ 1013 passed, 4 skipped; the skips are pre-existing environment skips)Coveragejob (uv run pytest --cov=openagent_eval --cov-fail-under=75→ 1067 passed, 81.12%)uv run ruff check .) — see note belowuv run mypy openagent_eval/) — see note belowA regression test drives a retriever whose
retrieve()raises, through the real pipeline, over a dataset item carrying acontext. It asserts the fallback still returns that context, that the exception is now visible in the log, and thatresult.errorsstays empty — that last assertion is deliberate, and the reason is below. Reverting the production change makes it fail; the failure output is theRuntimeErrorno longer appearing.Checklist
Additional Notes
On the choice you were asked to make in #256. The issue offered you two options — a log line only, or a log line plus an entry in the run's
errors— and said theerrorsentry "would be better still". I built that version first, and it is wrong. Recording a non-fatal degradation inerrorscorrupts the run's arithmetic:engine.py:83computessuccessful_evaluations = len(result.results) - len(result.errors). The existing defensive boundary inpipeline.pygets away with appending toerrorsbecause it also pushes a placeholder result, so the two cancel in that subtraction. A retrieval degradation has no such pairing — the item genuinely succeeds — so each one silently subtracts a success.Measured on a one-item run where retrieval degraded and the item still evaluated:
successful_evaluationsreported0when the true value was1, andfailed_evaluationsreported1when the true value was0. It also flows intosummary["errors"], into every renderer's failure section viareports/base.py, into the CLI'sErrors: N, and — the one that decided it — intocicd/plugin.py, which flattensfailed_evaluationsinto gate metrics. A user gating onfailed_evaluations == 0would start failing CI on a perfectly healthy run.So this PR is the log-only version. It trades none of that away and still delivers what #256 asked for: a broken retriever is now distinguishable from an empty one. The test pins
result.errors == []so that a future change reintroducing the entry is caught rather than quietly mis-counting.If you do want per-run structured visibility, I think it needs a channel no existing consumer reads as "failed item" — a
warningsordegradationslist onPipelineResultrather than an entry inerrors. Happy to send that separately if you'd like it.On the two unchecked test boxes.
ruff check .exits 1 on pre-existing findings atmainitself, so I could not honestly tick that. Instead I compared per-file, atmainand on this branch, for the files this PR touches:pipeline.pyhas the same two pre-existingTC001findings on both sides and the new test file is clean, so this diff adds nothing new. I have not reformatted or fixed those pre-existing findings, since that would bury a small behavioural change in unrelated churn. Themypybox is unticked for the same reason — it is not clean atmain, and this diff does not change that.Out of scope, noted rather than fixed:
_generatehas the same bare-exceptshape a few lines below. I left it alone to keep this diff to the issue, but it is the same class if you want it covered.Generated by Claude Opus 5 (brief, review), Kimi K3 (implementation)