feat(results): report coverage, judge agreement, and run limits - #289
feat(results): report coverage, judge agreement, and run limits#289Murali Chillakuru (mchillakuru) wants to merge 3 commits into
Conversation
Rows that could not be judged are dropped from every rate denominator, and nothing showed how many. That bias is directional rather than random: a provider content filter rejects the most adversarial transcripts, which are exactly the ones most likely to contain a real violation, so a run whose worst rows were filtered reports a low violation rate and reads as a pass. Adds compute_coverage() and format_coverage(). Coverage is written into metrics.json and printed immediately above the rates in the run headline, because a rate is only meaningful next to the denominator it was computed over. Above 10 percent excluded, a warning states the direction of the bias - the true rate is likely higher, not merely uncertain. infer_judge_status collapses every non-ok status to judge_failed, so the breakdown reads the raw judge_status field instead; otherwise filter_skipped and a genuine judge error are indistinguishable, and they mean different things. A test covers that. Also: a failed metrics.json write was logged at DEBUG, which lost the whole cost record while the run still reported success. It now warns and records metrics_write_failed on the manifest, so the gap is discoverable after the fact. Also: _detach_executor_from_atexit read executor._threads unguarded and reported failures at DEBUG. It runs on the graceful-teardown path, so a silent failure there means worker threads block process exit with nothing explaining why. The attribute is now guarded and failures warn. The Python 3.13+ absence of threading._shutdown_locks is expected and stays at DEBUG.
…ngerprint Multi-judge runs recorded a raw percent-agreement figure for the first dimension of each row. Raw agreement is not a reliability measure: violation rates are skewed, and on a skewed base rate two judges voting independently agree most of the time by chance alone, so a high figure can describe almost no real reliability. Adds fleiss_kappa() to analysis/stats.py with no new dependency, verified against the published Fleiss (1971) worked example to four decimal places rather than only against its own edge cases. None is treated as a real category, because a judge marking a dimension not-applicable took a position rather than leaving a gap. Kappa is computed at run level, not per row: it needs many items to estimate the marginal category distribution, so a per-row value would be degenerate. compute_judge_agreement pools votes across rows and reports per dimension, warning below the Landis and Koch 0.60 benchmark. The existing per-row agreement field is left as it was. Adds compute_judge_fingerprint() over judge model, prompt hash, dimension set and judge count, plus warn_if_judge_changed(). Swapping a judge model to cut cost moves rates on an unchanged target, and without this the shift is attributed to the target.
Every existing limit is per-call or per-task - max_tool_calls, max_turns, model timeout - so nothing bounded a run as a whole. The runtime watchdog dumps stacks and never terminates, which is diagnostics rather than a ceiling. The realistic failure is a typo, sample_size 5000 against an expensive judge, with nothing able to stop it. Adds an optional top-level limits: block with max_total_calls, max_total_tokens, max_wall_time_s and on_exceed. Enforcement sits in UsageAccumulator, which already observes every call. track_usage() is entered once per stage, so the accumulator carries a baseline from earlier stages; without it a run-level ceiling would reset at each stage boundary and never bind. A test covers that specifically. BudgetExceededError is deliberately not an LLM*Error: the provider call succeeded and this is the harness obeying the operator, so retry and fallback must not treat it as a transient fault. The runner catches it, keeps the normal per-stage bookkeeping so partial artifacts stay valid and readable, records stopped_by_limit on the manifest, and exits non-zero. No cost ceiling is offered. ASSERT has no pricing table and a limit derived from an invented one would be wrong in whichever direction the operator could least afford. REVERT TRAP, read before reverting: unknown top-level config keys are rejected outright, so once a user has written a limits: block, removing the key from the allow-list in config.py makes their config fail to load rather than fall back to unlimited. The allow-list entry carries a comment saying to keep it even if the enforcement is removed. Configs with no limits: block are unaffected.
Chang Liu (changliu2)
left a comment
Chang Liu (changliu2)
left a comment
There was a problem hiding this comment.
The three concerns here are real and the write-ups are unusually good. Coverage reporting and the fingerprint are solid and land additively. Two things block merge, both in the limits commit, plus two correctness issues in the kappa path that are cheap to fix.
The headline problem: the run ceiling is fail-open on exactly the path the PR is motivated by. BudgetExceededError is raised from UsageAccumulator.add(), which runs after the provider call returns, and the judge/inference/test_set per-row workers all swallow it in their broad except Exception. In the judge stage, tasks for every pending row are created up front, so after the ceiling trips, every remaining row is still called and paid for. Whether the run actually stops then depends on the 10% error tolerance: if the breach lands late enough that errored rows stay within tolerance, the stage succeeds, exit code is 0, and stopped_by_limit is never set — the ceiling silently did nothing. The sample_size: 5000 typo scenario is not bounded by this as written.
Verified against main: the full-suite failures are all pre-existing viewer tests (17/17 fail in test_viewer_server_artifacts.py on main too), so nothing here regressed. Manifest and metrics.json additions are strictly additive and None values are dropped, and the viewer reads named fields, so backward compatibility is fine. Kappa reproduces Fleiss (1971) at 0.2099 as claimed, and _record_usage sits outside _with_retries, so BudgetExceededError correctly avoids reclassification as a provider fault.
I'd like the four items below addressed before merge; the rest are optional.
Inline notes
assert_ai/core/model_client.py:249 — Critical: the ceiling is swallowed by per-row workers and does not bound consumption.
raise BudgetExceededError(...) fires from inside add(), on the call's return path. Its call sites are the per-row workers, all of which catch broadly: judge.py:330, inference.py:897, test_set.py:1080. Consequences: judge.py:397 creates a task for every pending row up front, so all remaining rows still issue their calls after the breach — overshoot is the whole remaining stage, not one call. judge.py:458-475: if the errored rows stay within the 10% tolerance, the stage completes, the runner never sees BudgetExceededError, manifest.stopped_by_limit is unset, exit code is 0. When it does surface, the affected rows are never written to scores.jsonl — weaker than the "partial artifacts stay valid" claim for this stage.
Suggested fix: check the ceiling before issuing a call rather than after recording it (a run-level tripped flag consulted at generate* entry), and make the error non-swallowable by adding BudgetExceededError to the re-raise clauses that already exist for LLMAuthError/LLMProviderError. Add a test that a breach inside a judge worker stops the stage and sets stopped_by_limit.
assert_ai/core/model_client.py:177 — High: calls with no usage payload are invisible to every ceiling.
if usage is None: return exits before self.calls += 1 and before check_limits(). Any provider response without usage — streamed, content-filtered, some tool paths — is uncounted and unchecked. Verified: acc = UsageAccumulator(limits=RunLimits(max_total_calls=1)); acc.add(None) x3 → calls == 0, exceeded() is None. This also makes max_wall_time_s weaker than documented: elapsed time is only evaluated when a usage-bearing call lands, so a run wedged in a hung target or a filtered-response loop never trips the wall clock.
Suggested fix: count the call and run check_limits() regardless of whether usage was returned.
assert_ai/analysis/stats.py:59 — High: single-item input is accepted and returns a structurally negative kappa.
The guard is if len(ratings) < 1, but the docstring says kappa is undefined for "fewer than two items." With one item, p_expected is estimated from that same item, so kappa is ≤ 0 by construction. Verified: fleiss_kappa([[True, False]]) == -1.0, and compute_judge_agreement on a one-row run emits "Low inter-rater agreement on 'policy_violation' (Fleiss kappa=-1.00 over 1 rows, 2 judges). The consensus verdict for this dimension is not a reliable one." That statement is an artifact of the estimator, not a property of the judges.
Suggested fix: if len(ratings) < 2: return None to match the docstring, with a test. Separately, consider a minimum item count before the warning is allowed to fire.
assert_ai/results.py:118 — Medium: mixed judge counts silently erase the agreement signal.
Vote lists are pooled per dimension regardless of length, and fleiss_kappa returns None on ragged input. Verified with 5 rows at 3 judges and 5 rows at 2 judges: {"kappa": None, "items": 10, "raters": 3, "low_agreement": False} and no warning. A run whose judge fan-out varied is indistinguishable from a run with no agreement problem, and low_agreement: false reads as reassurance.
Suggested fix: bucket ratings by rater count, compute on the largest bucket, and report the excluded count; when kappa is None, say why rather than emitting a silent null.
assert_ai/results.py:132 — Minor: "raters": len(ratings[0]) reports the first row's count. Misleading under the ragged case above. Fold into that fix.
assert_ai/core/model_client.py:173 — Minor: warn mode logs once per stage, not once per run. _limit_warned lives on the per-stage accumulator, and track_usage() is entered once per stage, so on_exceed: warn produces one warning per stage. Either carry the flag alongside the baselines or adjust the docs.
docs/config/schema.md (limits section) — Minor. "the run ends with a non-zero exit code, manifest.json records stopped_by_limit" is not reliably true today given the first comment. Worth landing the enforcement fix before the doc claim.
tests/test_run_limits.py — Minor: the wiring is untested. Every enforcement test constructs UsageAccumulator directly. Nothing covers a limits: block travelling through load_runtime_context into ctx["limits"] and reaching track_usage(...) at runner.py:849-857, nor the cross-stage baseline handoff at runner.py:941-943 in a real stage loop.
Verdict: Request Changes — the run-limit enforcement is fail-open on the judge path it exists to protect, and can let a breached run exit 0 with no record of the breach.
Must fix before merge
BudgetExceededErrorswallowed by per-rowexcept Exceptionin judge/inference/test_set; consumption continues to end of stage; breach can be tolerated silently and exit 0.- Calls returning no usage bypass counting and all ceiling checks, incl. wall time.
fleiss_kappaaccepts a single item and warns "low agreement" on a structurally forced −1.0.- Ragged rater counts yield a silent
kappa: null/low_agreement: false.
Nice to have
- Per-run rather than per-stage dedupe for
on_exceed: warn. ratersfield accuracy under ragged input.- Temper the schema.md exit-code/artifact claims until #1 lands.
- Integration test for config →
ctx["limits"]→track_usageand cross-stage baselines.
Verified as non-issues: Manifest/metrics.json additions are additive with None dropped; viewer consumes named fields only — no consumer break; coverage handles the empty-row and all-excluded cases; the Fleiss worked example reproduces; BudgetExceededError is raised outside _with_retries so retry/fallback never reclassifies it; full-suite failures are pre-existing viewer-test failures present on main.
2 participants
Summary
Three commits, all answering the same question: can the numbers this run reports be
trusted?
Coverage. Rows that could not be judged are dropped from every rate denominator, and
nothing showed how many. The bias is directional rather than random — a content filter
rejects the most adversarial transcripts, which are exactly the ones most likely to
contain a real violation — so a run whose worst rows were filtered reports a low
violation rate and reads as a pass. Coverage now appears in
metrics.jsonand directlyabove the rates, with a warning that states the direction of the bias.
Agreement. Multi-judge runs recorded raw percent agreement for one dimension of each
row. Raw agreement is not a reliability measure: violation rates are skewed, and on a
skewed base rate two judges voting independently agree most of the time by chance alone.
Adds Fleiss' kappa with no new dependency.
Limits. Every existing limit is per-call or per-task, so nothing bounded a run as a
whole; the watchdog dumps stacks and never terminates. The realistic failure is a typo —
sample_size: 5000against an expensive judge — with nothing able to stop it.Closes: SEC-011 (content-filter survivorship bias) · SEC-022 (judge verdict treated as authoritative) · SEC-019 (judge changes reach results ungated) · SEC-018 (no consumption ceiling) · AP-006 · AP-007
Commits
Testing
Full suite green on this branch;
11 files changed, 1049 insertions(+), 12 deletions(-). Every change has a regression test, and thesuite was re-run after each commit rather than only at the end.
Notes for reviewer
Please read before merging — this PR contains a one-way door.
Unknown top-level config keys are rejected outright. Once a user has written a
limits:block, removing the key from the allow-list in
config.pymakes their config fail toload rather than fall back to unlimited. The allow-list entry carries a comment saying
to keep it even if the enforcement is removed. Configs with no
limits:block areunaffected. If that trade is not wanted, drop the third commit.
Kappa was validated against the published Fleiss (1971) worked example — 10 subjects,
14 raters — reproducing 0.2099 to four decimal places, rather than only against its own
edge cases. It is computed at run level, not per row: it needs many items to estimate the
marginal category distribution, so a per-row value would be degenerate. The existing
per-row
agreementfield is left as it was.Noneis treated as a real category, sincea judge marking a dimension not-applicable took a position rather than leaving a gap.
There is deliberately no cost ceiling. ASSERT carries no pricing table, and a limit
computed from an invented one would be wrong in whichever direction the operator could
least afford. Token, call and wall-time ceilings are directly measurable and are offered
instead.
BudgetExceededErroris deliberately not anLLM*Error: the provider call succeeded andthis is the harness obeying the operator, so retry and fallback must not treat it as a
transient fault. Partial artifacts stay valid and readable after a stop.
Also folded in: a failed
metrics.jsonwrite was logged at DEBUG, losing the entire costrecord while the run reported success (AP-007); and
_detach_executor_from_atexitread aprivate attribute unguarded and reported failures at DEBUG on the graceful-teardown path
(AP-006).
Risk and rollback
Each commit is a single concern and can be reverted independently. See the notes above
for anything that does not revert cleanly.