fix: count a job-reported error as a failed row

[Baukebrenninkmeijer]

The bug

evaluatorq reported Failed Jobs 0, Success Rate 100%, ✓ Evaluation completed successfully and exit 0 on a run where the conversation never happened.

Reproduced while running the example on the docs/autofill-wrap-simulation-agent page with an ORQ_API_KEY that is invalid for its workspace. The simulation got 401 authentication_error, SimulationRunner.run caught it and returned terminated_by=error, and the run reported a full pass over a dead target.

Why nothing saw it

• simulation/runner.py swallows the exception on purpose and returns a partial result. Correct — one bad row must not kill the batch.
• simulation/convert.py:157 already records the failure: 'error': {'message': result.reason}, plus 'status': 'failed'. The signal existed.
• But it is nested inside output. wrap_agent.py returned {'name', 'output'} with no top-level error, and processings.py read only result['name'] and result['output'].
• So JobResult.error was set only when the job raised, and table_display.py:47 counts failures from that field. Hence 100%.

The fix

1. process_job honours a top-level error key returned by a job, and keeps the output — unlike the raise path, which discards it. The transcript is the most useful thing you have when diagnosing a failed run.
2. Both simulation jobs — simulate()'s own (_build_simulation_job_and_cache) and wrap_simulation_agent's — emit error unconditionally: None on success, the runner's reason when it ended in error or timeout. Both derive it through one helper, simulation.evaluators.scorers.failure_reason, so the same dead run cannot describe itself two ways depending on the entry point.
3. Presence is the failure signal, not the message's truthiness. A payload that flattens to nothing becomes job reported a failure with no readable message and logs a WARNING, rather than silently becoming a clean row again.
4. Flattening delegates to common.output_adapters.output_error_text, the repo's existing reader of an error payload, so a job-level and an output-level error cannot disagree about what the same dict means.
5. The job's OTel span is marked ERROR on every failed row, including the two raise paths — those previously closed as OK while the summary table called the row failed.

Note this is a different key from the one CLAUDE.md's "Target calls and error payloads" section legislates. That one lives inside the output payload of a target or LLM invocation and decides what a judge reads; this one is at the top level of a job's return value and decides whether the row counts as failed. An invocation lives inside a job, so a job can carry both. @job() nests what it is handed under output, which is where the judge-facing key belongs — a decorated job reports a row failure by raising. CLAUDE.md now spells the distinction out, because this review got it wrong first.

What this does not change

Library-level exit codes. Failed Jobs and Success Rate now move by default, and simulate(exit_on_failure=True) — its default — now raises for a run that ended in error or timeout, not only for a dropped row. evaluatorq() itself still never exits: it has no gate of its own, and check_pass_failures's treat_errors_as_failure still defaults to False. A caller who never opted in gets the same exit code as before; the docs now show the flag and say what leaving it off means.

JobReturn is documentation, not enforcement. Job types a job's return as dict[str, Any], and a TypedDict is not assignable to that, so annotating producers -> JobReturn does not type-check. Tightening the alias would break every existing user job. The docstring now states this rather than implying otherwise.

Compatibility

A job already returning a top-level error key for a non-failure reason will now be counted as failed. Nothing in this repo does; CHANGELOG.md notes it under ### Notable defaults.

Docs

• docs/guides/simulation-in-evaluatorq.md taught the workaround for this exact bug ("evaluatorq reports a 100% success rate for this run even when the conversation itself never happened"). The example now gates on job_result.error.
• docs/evaluation-reference.md documents the raw-dict job contract — emit error on every path, None on success — and its CI-gate snippet now passes treat_errors_as_failure=True, with the default's behaviour stated.

Tests

All fakes, no credentials:

• tests/unit/test_processings.py — a reported error sets JobResult.error, keeps the output, is still scored, and trips check_pass_failures(treat_errors_as_failure=True); an omitted key and an explicit None both stay clean; a blank or unreadable payload reports the placeholder rather than None; and create_summary_display reads Failed Jobs 1 / Success Rate 0% for such a row, which is the claim the CHANGELOG makes.
• tests/simulation/test_wrap_agent.py — error/timeout report the reason; judge/max_turns emit the key as None rather than omitting it; a failure with an empty reason still reports one.
• tests/simulation/test_simulate_job_error_key.py — the same contract on simulate()'s own job.
• tests/simulation/test_hooks.py — exit_on_failure=True raises for a run that ended in error; False warns and returns the partial results.

CI checks verbatim: ruff check src, ruff format --check src, basedpyright (0 errors, whole repo), pytest -m 'not integration' (5252 passed, 2 skipped), mkdocs build --strict.

🤖 Generated with Claude Code

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
src/evaluatorq
processings.py
table_display.py
types.py
src/evaluatorq/common
output_adapters.py
src/evaluatorq/integrations/langchain_integration
wrap_agent.py
src/evaluatorq/simulation
api.py
wrap_agent.py
src/evaluatorq/simulation/evaluators
scorers.py
Project Total

This report was generated by python-coverage-comment-action

[currentlycodinng]

Verified at cd0a105. ruff check src, ruff format --check src and basedpyright all clean; pytest -m 'not integration' gives 5161 passed, 4 skipped locally, and gh pr checks 184 is 13/13.

The bug is real and the diagnosis is right. cd0a105^:processings.py reads result['name'] at 159 and result['output'] at 160 and nothing else, and table_display.py:47 counts failures off job_result.error, so a job that caught its own failure was invisible. wrap_agent.py:99 flags exactly {error, timeout}, which matches UNEVALUATED_TERMINATIONS at scorers.py:20; judge and max_turns correctly stay None. I checked the compatibility claim by grepping src/, examples/ and docs/ and it holds: every other 'error': key is nested in an output payload, a report field or a colour map, so nothing in the repo starts failing.

I probed _job_reported_error directly rather than trusting the docstring:

{'message': 'x', 'code': 123} -> 'x'
{'weird': ..., 'no_message_key'} -> "{'weird': 'shape', ...}"
{'message': '', 'detail': 'fb'} -> 'fallback'
'boom' -> 'boom' '' -> None None -> None
False -> 'False' 0 -> '0'

Two notes on that below, inline.

The larger point, which I can't attach inline since these files aren't in the diff: simulate() has the same bug and this PR does not fix it. simulation/api.py:2102 returns {'name': job_name, 'output': to_open_responses(...)} with no error key, and api.py:2318 hands it straight to evaluatorq(jobs=[job_fn]). Four lines up, api.py:2098 already computes terminated_by in (error, timeout) for the hook, so the branch is sitting right there. It's less bad than the wrap path because the default scorers catch it (criteria_met scores 0.0 at scorers.py:206 and api.py:2185 reports pass=False), so there's no silent 100%. But Failed Jobs still reads 0 on that path and check_pass_failures(treat_errors_as_failure=True) still misses those rows. integrations/langchain_integration/wrap_agent.py:148-151 is the same shape (returns name/output only, no error key), and CLAUDE.md:190 says a job that calls a target must emit the key unconditionally. Also worth a follow-up: types.py:140-144's JobReturn TypedDict still only declares name and output, and it's exported from init.py, so a user typing their job against it can't legally add the key the new contract wants without a NotRequired[str | None] there.

Not blocking. Happy for the sibling paths to be a follow-up if you'd rather keep this diff to the mechanism plus one caller, but I'd rather they went in here while the reasoning is loaded.

[Baukebrenninkmeijer]

Review — pr-review-stolen + hate --codex

11 critics over origin/main...HEAD: seven review lenses (code, minimal, comments, tests, errors, types, spec) and four hate critics (Skeptic, Pedant, Historian, Realist) plus an Outsider run through the Codex CLI on gpt-5.6-luna. Repo checks were green before and after: ruff check src, ruff format --check src, basedpyright (whole repo, 0 errors), pytest -m 'not integration' (5247 passed), mkdocs build --strict.

Critic	Lens	Rating
The Skeptic	strategy	🟢 MINOR ISSUES
The Pedant	quality	🟠 NEEDS REWORK
The Historian	reuse	🟠 NEEDS REWORK
The Realist	goal	🟠 NEEDS REWORK
The Outsider	all four, via Codex	🔴 FUNDAMENTALLY FLAWED

Applied in 2a883ff

1. A blank error message flattened back to a clean row. Eight critics, independently — the highest-agreement finding in the run, and the one that mattered. _job_reported_error mapped '', {'message': ''} and {'message': None} to None, so a job that reported a failure with nothing to say produced a passing row: exactly the bug this branch exists to close, reintroduced for one payload shape, and pinned by this branch's own tests. It was reachable — wrap_agent passed result.reason straight through, and the runner builds that reason as str(e), which is empty for any exception raised without arguments. Presence is now the failure signal: an unreadable payload becomes job reported a failure with no readable message and logs a WARNING, and failure_reason falls back to the termination when the reason is empty.

2. A fourth error-flattening ladder. The Historian and the Outsider both found common/output_adapters.py:49's output_error_text, which already flattens a str-or-dict error payload and already handles the present-but-empty case correctly ("Error presence, rather than message truthiness, marks a failed target"). The minimality lens reached the same place from the other direction — no producer in the repo emits a dict, so the message/error/detail ladder, the JSON fallback and the json import existed for a caller that does not exist. Flattening now delegates to output_error_text, and the helper is 8 lines instead of 27. Note the ceiling: a dict with no readable message now renders as output_error_text renders it everywhere else, rather than picking detail out. No producer emits that shape.

3. Two derivations of "why did this simulation fail". Six critics. simulate()'s job read metadata['error'] or reason; wrap_simulation_agent's read reason alone. They agree today only because _partial_result sets both from the same string — an invariant enforced in a third file neither call site references. Both now call simulation.evaluators.scorers.failure_reason, beside UNEVALUATED_TERMINATIONS.

4. The job span stayed green on a failed row. The Skeptic and the code lens. set_span_error exists in common/tracing.py for exactly this ("For swallowed failures — the code recovered, but the span should not read as OK in a trace viewer") and was unused here, so the summary table said Failed Jobs 1 while the trace said OK. All three failure paths in process_job now mark the span, including the two raise paths that had the same gap.

5. Docs that contradicted the code. The Realist, the spec lens and the Outsider. docs/guides/simulation-in-evaluatorq.md:132 still taught the workaround — "evaluatorq reports a 100% success rate for this run even when the conversation itself never happened" — on the exact page the bug was found on. The PR body deferred that to #180, which has already landed. Fixed here: the example now gates on job_result.error. docs/evaluation-reference.md gains the raw-dict job contract (emit error on every path, None on success; @job() nests the key under output, so a decorated job reports failures by raising) and its CI-gate snippet now passes treat_errors_as_failure=True with the caveat stated.

6. Comment and docstring corrections. The comments lens and the Pedant. The same paragraph appeared five times; the check_pass_failures addition and the Job alias docstring were false as written ("returns a dict with 'name' and 'output' keys", one line under the TypedDict that had just gained a third key). JobReturn's docstring now says what is actually true — including that Job types the return as dict[str, Any], so nothing type-checks a job against the shape.

7. Test gaps. The tests lens and the Outsider: nothing asserted the user-visible claim. create_summary_display had zero coverage in the repo, and the CHANGELOG's claim was written in terms of Failed Jobs. Added a test that Failed Jobs reads 1 and Success Rate reads 0% for an error-reporting row, and a producer-level test that a terminated_by=error run with an empty reason still reports one.

Open — needs a call

1. The bug report said "exit 0" and this branch does not change any exit code. Five critics landed on this. Failed Jobs and Success Rate do change by default — table_display.py:47 reads job_result.error with no gate. But evaluatorq() never calls check_pass_failures itself, treat_errors_as_failure defaults to False, ✓ Evaluation completed successfully is an unconditional console.print, and simulate(exit_on_failure=True) only raises for rows missing from the result cache — an error row is in the cache, so it does not fire. The repro (invalid ORQ_API_KEY, 401, terminated_by=error) still exits 0.

• A — make an errored row fail by default: flip treat_errors_as_failure to True, and fold error rows into simulate()'s exit_on_failure decision. Costs a real default flip: every existing script that currently exits 0 over a dead row starts failing, so it needs a feat!-level conversation or a deliberate opt-out flag.
• B — leave the defaults, say so plainly: the CHANGELOG now states that exit codes are unchanged and the docs now show treat_errors_as_failure=True. Costs the gap staying open for anyone who does not read either.

Lean: B for this branch, A as its own PR. The counters are the honest scope of a fix:; changing when a run exits non-zero is a separate decision with a blast radius, and merging it in here would hide it behind a bug fix.

2. @job() cannot emit the key the new contract requires. job_helper.py:84 wraps a function's return into {'name', 'output'} and never lifts an inner error to the top level, so every decorated job — the way the docs teach job authoring, and how red team and the integrations build theirs — reports failures only by raising. CLAUDE.md tells the next author "a new job that calls a target must emit the error key unconditionally" while the sanctioned helper makes that impossible.

• A — make the contract reachable: give @job an explicit way to return a handled failure (a JobOutcome return type, or a documented sentinel), and audit redteam/adaptive/pipeline.py, redteam/runner.py and integrations/langchain_integration/wrap_agent.py:148, which the Outsider and the errors lens found still nest their error under output. Costs a decorator change plus a repo-wide sweep, in a PR that is currently about process_job.
• B — document the split: @job jobs raise; raw-dict jobs may report. Already written into docs/evaluation-reference.md in the applied commit.

Lean: B now, A as a follow-up ticket. The Realist checked the other builders and found their failures already raise or already flow through red team's own error_payload() system, so this is a completeness gap rather than a live second instance of the bug — but it is a real one, and CLAUDE.md's rule is currently untrue as written.

3. JobReturn enforces nothing. Four critics. Job = Callable[[DataPoint, int], Awaitable[dict[str, Any]]], so the TypedDict is documentation. I tried annotating both simulation producers -> JobReturn and basedpyright rejected it: a TypedDict is not assignable to dict[str, Any], so the producers no longer satisfy the Job alias. Tightening the alias itself makes every existing user job a type error — a breaking change. Reverted; the JobReturn docstring now says the contract is unenforced rather than implying otherwise.

Rejected

• The dict-key ladder's message/error/detail precedence. The Outsider called it wrong ({'message': None, 'detail': 'real'} discards the detail). True, but moot — the ladder is gone, and output_error_text keeps the whole payload in that case.
• error: False counts as a failure with the message 'False'. Two critics wanted it treated as "no error". False is outside the declared str | None contract, and counting an unreadable value as failed is the conservative direction; the CHANGELOG documents it.
• Test quote-style inconsistency. The Pedant checked each file's pre-existing convention and confirmed both new blocks match their own file. ruff format is scoped to src deliberately. Not a finding.
• The third UNEVALUATED_TERMINATIONS site (api.py:2239). Inside _sim_evaluation_details; it derives an evaluator explanation, not a job return, and already reports pass_=False. No change needed.
• test_simulate_job_error_key.py monkeypatching runner._run_with_timeout. Flagged as reaching past the public seam, but that private method is what the production path calls (api.py:2152, with its own noqa); patching run would intercept nothing.

Solo flags, not applied

• Evaluator averages in the detailed table now include an errored row's scores, so one panel can read 100% while the summary counts the row failed — newly possible because an errored row keeps its scores. Worth a look.
• to_open_responses writes output['error'] only for terminated_by == 'error', not timeout, so a timed-out run's nested error field is still None for any judge reading it.
• No log line at the choke point in process_job when a job reports an error and no results table is printed — both red team and simulation pass print_results=False.

[Baukebrenninkmeijer]

Took the sibling paths here rather than deferring — the reasoning was loaded, as you say. Rebased onto main and force-pushed; head is now b34202da.

• simulate(): api.py:2162 returns 'error': reason, reusing the reason it already computes for on_datapoint_error four lines up. New tests/simulation/test_simulate_job_error_key.py covers both branches — set on a runner error, present-as-None on a judged run.
• JobReturn: now declares error: NotRequired[str | None] with a docstring saying when to emit it, so a user typing their job against the exported TypedDict can satisfy the contract.
• UNEVALUATED_TERMINATIONS is imported in both wrap_agent.py and api.py; the literal tuple and the local TerminatedBy import are gone.
• check_pass_failures' docstring is corrected (evaluatorq.py:56).

The one I did not change is integrations/langchain_integration/wrap_agent.py. It has no try/except anywhere — agent.ainvoke raises straight through and process_job records it — so 'error': None there could never be anything but a literal. CLAUDE.md's rule is aimed at jobs that catch their own failures; I read that path as out of scope rather than fixed. Say the word if you want the key there for uniformity.

Left open: the evaluators-run-on-an-errored-row thread, where I documented the consequence rather than skipping the scorers. Reasoning is on that thread.

CI verbatim: ruff check src, ruff format --check src, basedpyright whole repo 0 errors, pytest -m 'not integration' 5245 passed / 2 skipped.

[Baukebrenninkmeijer]

Review decisions

Both open decisions from the review above are resolved and applied in 39f014f.

1. exit_on_failure now covers a run that ended in error or timeout

Decided: when the caller already asked to exit on a failure, a dead run is one. No default was flipped — check_pass_failures(treat_errors_as_failure=...) is untouched, and a caller who never opted in gets the same exit code as before.

The check counted rows missing from the result cache. A run the runner ended in error is in the cache — it produced a result, just not a conversation — so simulate() and eq sim returned normally over a target that was 401 throughout, which is exactly what exit_on_failure=True exists to catch. SimulationDroppedError's message now names both counts: dropped rows and rows that ended in error or timeout. exit_on_failure=False is unchanged — same rows, reported as a WARNING, partial results returned. Scorer verdicts stay reporting-only, so an agent that answered badly still does not raise.

Two existing tests whose subject is something else — the terminal hook firing, and the per-simulation wall clock — now opt out explicitly rather than relying on a dead row exiting 0.

2. @job() not carrying a top-level error key is the contract, not a gap — I had this wrong

The rule in CLAUDE.md ("a new job that calls a target must emit the error key unconditionally") is about the key inside the output payload of a target or LLM invocation, which is what a judge reads. An invocation lives inside a job; it is not the job's top level. JobReturn['error'] is a different key that decides whether the row counts as failed. @job() nesting what it is handed under output puts that key exactly where the judge-facing rule wants it.

So there is no repo-wide sweep to do, and the red team and integration paths are not carrying a latent instance of this bug — three critics said otherwise and were wrong, my synthesis included. CLAUDE.md now carries the distinction in the same section, so the next reader does not repeat it, and the wording in docs/evaluation-reference.md was corrected to say where a decorated job's error key lands and what reads it.

3. JobReturn staying unenforced

Confirmed as-is. Job types a job's return as dict[str, Any], a TypedDict is not assignable to that, and tightening the alias would break every existing user job. The docstring states this rather than implying enforcement.

Also landed on this branch while the review ran

a48037fb (parallel session): a row whose job reported its own failure now skips its evaluators rather than scoring a transcript already known to be dead — an LLM judge call per row for a verdict on a conversation that never happened. It keeps its output for diagnosis. wrap_langchain_agent reports a failing invoke/ainvoke through the top-level key instead of raising. The docstrings and docs this review touched were updated to match, so the "still scored" wording above is superseded by that commit, not by this one.

Green on the merged state: ruff check src, ruff format --check src, basedpyright (whole repo), pytest -m 'not integration' (5252 passed, 2 skipped), mkdocs build --strict.

[currentlycodinng]

Re-verified at 39f014f. ruff check src, ruff format --check src and basedpyright clean, pytest -m 'not integration' gives 5236 passed / 4 skipped locally, gh pr checks 184 is 13/13.

Every point from my last pass is addressed, and two of them better than I suggested. simulate()'s job at api.py:2158 and the langchain job at wrap_agent.py:165 both emit the key unconditionally, types.py:151 types it NotRequired[str | None], wrap_agent.py:104 goes through failure_reason so the two simulation paths cannot describe the same dead run differently, and evaluatorq.py:53 no longer claims an errored row has no scores. The falsy-message and repr complaints are answered by routing through output_error_text and making presence the signal rather than truthiness; I checked {'message': ''} and '' both reach the placeholder with a WARNING carrying the payload. Skipping evaluators on an errored row removes the judge-call cost entirely, which is a better answer than the readability fix I asked for. I confirmed the skip live: a dead target logs 'skipping 2 evaluator(s) for row 0' and no scorer runs.

The langchain try/except earns more than the changelog says. processings.py's generic except Exception never sets job_name, only the JobError path does, so a raising raw job used to land in the summary table as 'job'. Returning the error keeps the real name.

One thing I want an answer on before this merges. Widening exit_on_failure to error and timeout rows also throws away the run's artifacts. _simulate_core re-raises at api.py:1624, so build_simulation_run (:1651), the executive summary, the recommendations and auto_save_run (:1706) are all skipped, and cli.py:783 exits 1 before _write_results or _export_report. When only dropped rows triggered this, fine. Now a single provider timeout in a 50-row batch discards the saved run, the report and the results file for the 49 rows that worked, and the caller paid for those. Either save before raising, or keep error/timeout on the warning path by default and let a caller opt into the hard gate.

Two smaller things, neither in this diff so noting here instead of inline. The docs updates missed three places that still describe the dropped-only semantics: src/evaluatorq/simulation/README.md:75, README.md:183, and the SimulationDroppedError docstring at simulation/exceptions.py:20 ("Raised when simulation job(s) produced no result and were dropped"), which now also raises for rows that did produce a result and just ended in error or timeout. And skipping evaluators means a dead simulate row carries no scores at all, I probed one and its metadata is ['datapoint_id', 'error', 'persona', 'scenario'] with no evaluator_scores, where criteria_met used to stamp 0.0. That makes scorers.py:222 and api.py:2246 unreachable from both entry points, which is where RES-1308 put the guard against an unaudited run showing green in the uploaded experiment. The row still uploads with error set, so this is a question about how the experiment renders a scoreless row rather than a bug.

[currentlycodinng]

This now fires for any row that ended in error or timeout, and the raise skips everything downstream: build_simulation_run at :1651, the executive summary, the recommendations, and auto_save_run at :1706. cli.py:783 catches it and exits 1 before _write_results and _export_report. So one timed-out row in a 50-row batch discards the saved run and the report for the 49 that worked, and those were paid for. Save the run before raising, or leave error/timeout on the warning path by default.

[currentlycodinng]

Consequence worth confirming: a dead simulate row now reaches the uploaded experiment with no evaluator scores at all. I probed one and its metadata is ['datapoint_id', 'error', 'persona', 'scenario'], criteria_met used to stamp 0.0 here. That makes scorers.py:222 and api.py:2246 unreachable from simulate() and wrap_simulation_agent, and those branches exist so an unaudited run cannot show green in the experiment (RES-1308). The row does carry error, so this may be fine, I want to know it was a decision.