From 3672a2897530f701596b7160a720a882e875720d Mon Sep 17 00:00:00 2001 From: Colinho22 <48288595+Colinho22@users.noreply.github.com> Date: Tue, 15 Sep 2026 20:03:20 +0200 Subject: [PATCH 1/3] feat: classify failure modes for invalid generations Adds a failure taxonomy so the reliability side of the harness is analysed as carefully as the accuracy side. Previously the entire failure surface reduced to one count, which cannot say why a strategy is less reliable. analysis/failures.py holds FailureCause and classify_failure, resolving each invalid run to one primary cause under a fixed precedence. Causes are not mutually exclusive in practice (a truncated response is usually also a parse error), so a single primary cause keeps rates summable rather than multi-labelling them. An unmatched error string surfaces as UNKNOWN rather than joining a real category. db/queries.py gains fetch_failure_rows. The failing text is read from the first failed sub_results row, not run_results: on a failed run the top-level raw_response is always NULL because the error result is built before any text exists. A correlated subquery keeps the grain at one row per run instead of multiplying by sub-result. statistics.py gains failure_rates (pooled counts by strategy, model and tier) and survivor_bias (the gap between valid_only and intent_to_treat per strategy). Rates are pooled rather than per-cell means: a per-cell mean would weight a one-run cell as heavily as a five-run cell, which for a rate is the wrong grain. Classifies all 478 failures in the existing corpus with zero UNKNOWN, and retroactively, with no model re-invocation. The failure-rate figure is deliberately left to the figure-generation work, which is sequenced after this. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013jLSXwVs1RmfUbCUbQurCJ --- docs/analysis.md | 127 ++++++++ src/maestro/analysis/__init__.py | 10 + src/maestro/analysis/__main__.py | 20 ++ src/maestro/analysis/failures.py | 188 ++++++++++++ src/maestro/analysis/statistics.py | 186 +++++++++++- src/maestro/db/queries.py | 50 ++++ tests/analysis/test_failures.py | 446 +++++++++++++++++++++++++++++ 7 files changed, 1025 insertions(+), 2 deletions(-) create mode 100644 src/maestro/analysis/failures.py create mode 100644 tests/analysis/test_failures.py diff --git a/docs/analysis.md b/docs/analysis.md index ce82365..60d8869 100644 --- a/docs/analysis.md +++ b/docs/analysis.md @@ -50,6 +50,8 @@ output/analysis/20260621T111935Z/ effect_sizes.json error_taxonomy_by_strategy.json tradeoff_correctness_efficiency.json + failure_rates.json + survivor_bias.json figures/README.md ``` @@ -140,6 +142,131 @@ Per-strategy medians on both dimensions: `entity_id_f1` (correctness) and `cost_usd` / `duration_ms` (efficiency). Consumed by the dashboard's Pareto view. +### 3.9 `failure_rates.json` + +Failure rate and cause breakdown per strategy, model, and tier, plus an +`overall` block. This is the reliability counterpart to the accuracy +tables: `error_taxonomy_by_strategy.json` scores the content of diagrams +that were produced, while this file covers the runs that produced nothing +scorable at all. The two populations are disjoint. + +Rates are **pooled counts**: the denominator is every run attempted in the +group, not a mean of per-cell rates. A per-cell mean would weight a cell +with one run as heavily as a cell with five, which for a rate is the wrong +grain. This deliberately differs from the F1 path, which aggregates per +cell because it averages a score rather than counting events. + +Every cause appears in every `causes` block, including the zeros: an +absent key would be ambiguous between "never happened" and "not measured". +Controls are excluded (they never call a model). + +Payload shape (abridged): + +```json +{ + "status": "ok", + "statistic": "pooled_failure_rate", + "overall": { + "n_runs": 6000, + "n_failed": 478, + "failure_rate": 0.0797, + "causes": { "schema_violation": 261, "truncation": 135, "...": 0 } + }, + "by_strategy": [ + { + "strategy": "sop_based", + "n_runs": 1500, + "n_failed": 151, + "failure_rate": 0.1007, + "causes": { "schema_violation": 85, "...": 0 } + } + ] +} +``` + +### 3.10 `survivor_bias.json` + +Per strategy, the primary DV under `valid_only` (survivors only) against +`intent_to_treat` (every run, failures scored 0.0). The gap between them +is the survivor bias: a strategy that fails often looks better under +`valid_only` because its failures were dropped rather than scored, and +`survivor_bias` measures exactly how much that dropping flatters it. + +Reported per strategy rather than as one pooled figure, because the bias +scales with each strategy's failure rate: a single number would hide the +comparison that matters. `n_cells_dropped` counts cells where every run +failed, so `valid_only` had nothing to average. + +--- + +## 3a. Failure taxonomy + +`failure_rates.json` classifies each invalid run into exactly one primary +cause. The classifier lives in `src/maestro/analysis/failures.py`. + +### What counts as a failure + +Two disjoint shapes, both rejected by `RunResult.success`: + +- an **errored** run: `error` is set, and the string is the evidence. +- a **silent-empty** run: `error` is `None` but the diagram is missing or + blank. A provider can return whitespace without raising, so this is a + real category, not a data defect. + +### Where the evidence lives + +On a failed run, `run_results.raw_response` is always `NULL`: the error +result is built before any text exists. The failing text is retained one +level down, on the `sub_results` row that failed. Classification therefore +reads the run's `error` plus the first failed sub-result's `raw_response`. + +This means historical runs can be classified retroactively with no model +re-invocation, which is why re-scoring was never needed for this analysis. + +### The causes + +| Cause | Meaning | +|---|---| +| `rate_limit` | Provider returned a rate-limit error. | +| `timeout` | Request exceeded the client deadline. | +| `api_error` | Generic provider API error (the SDKs' catch-all base class). | +| `safety_block` | Response blocked by a content or safety filter. | +| `empty_output` | Provider returned no text, or only whitespace. | +| `truncation` | Response stopped mid-structure, consistent with a token limit. | +| `parse_error` | Output was not parseable in the requested format. | +| `schema_violation` | Output parsed but broke the Mermaid output contract. | +| `orchestration_error` | The framework misbehaved, not the model output. | +| `unknown` | No rule matched. Visible by design rather than mis-filed. | + +### Classification rules + +Causes are not mutually exclusive in practice: a truncated response is +usually *also* a parse error, because the truncation is what broke the +parse. Rather than multi-label (which makes rates hard to sum and +compare), each failure gets one primary cause under a fixed precedence, +most specific first: + +1. **Infrastructure** (`rate_limit`, `timeout`, `safety_block`). If the + API never returned, nothing downstream is meaningful. +2. **`empty_output`**. A missing response cannot be a parse error. +3. **`schema_violation`**, before the generic parse rule: well-formed text + that violates the output contract is a different failure from text that + is not parseable at all. +4. **`parse_error`**, promoted to **`truncation`** when the parser message + is truncation-shaped (an unterminated string, or a structure that simply + stops) *and* the raw response is long enough that running out of tokens + is plausible. Without the raw text the two are indistinguishable, so the + conservative `parse_error` label stands: under-reporting truncation is + safer than inventing it. +5. **`orchestration_error`**. +6. **`api_error`** last among the infrastructure family, since a more + specific subclass above must win over the catch-all base class. +7. **`unknown`** as the explicit fallback. + +Adding a provider means checking whether its error prefixes are covered. +An unmatched prefix surfaces as `unknown` rather than being mis-filed, +which is the failure mode this ordering exists to prevent. + --- ## 4. `report.md` diff --git a/src/maestro/analysis/__init__.py b/src/maestro/analysis/__init__.py index 5f74eb1..0384d69 100644 --- a/src/maestro/analysis/__init__.py +++ b/src/maestro/analysis/__init__.py @@ -3,6 +3,7 @@ # Statistical analysis pipeline. Re-exported so callers can do # ``from maestro.analysis import describe, anova_strategy`` without reaching # into the submodule. The CLI lives in maestro.analysis.__main__. +from maestro.analysis.failures import FailureCause, classify_failure from maestro.analysis.statistics import ( DEFAULT_CONVENTION, INTENT_TO_TREAT, @@ -16,9 +17,12 @@ describe, effect_sizes, error_taxonomy_by_strategy, + failure_rates, load_dataframe, + load_failure_dataframe, mixed_effects_robustness, posthoc_strategy, + survivor_bias, tradeoff_correctness_efficiency, ) @@ -51,8 +55,14 @@ "describe", "effect_sizes", "error_taxonomy_by_strategy", + "failure_rates", "load_dataframe", + "load_failure_dataframe", "mixed_effects_robustness", "posthoc_strategy", + "survivor_bias", "tradeoff_correctness_efficiency", + # failure classification + "FailureCause", + "classify_failure", ] diff --git a/src/maestro/analysis/__main__.py b/src/maestro/analysis/__main__.py index 41f2a06..09de3b8 100644 --- a/src/maestro/analysis/__main__.py +++ b/src/maestro/analysis/__main__.py @@ -32,9 +32,12 @@ describe, effect_sizes, error_taxonomy_by_strategy, + failure_rates, load_dataframe, + load_failure_dataframe, mixed_effects_robustness, posthoc_strategy, + survivor_bias, tradeoff_correctness_efficiency, ) from maestro.analysis.timestamps import format_for_display @@ -59,8 +62,14 @@ ("descriptive.json", describe), ("error_taxonomy_by_strategy.json", error_taxonomy_by_strategy), ("tradeoff_correctness_efficiency.json", tradeoff_correctness_efficiency), + ("survivor_bias.json", survivor_bias), ] +# failure_rates is wired separately rather than added to _ANALYSES: it needs +# the failure frame as well as the run frame, so it does not share the +# single-DataFrame signature every entry above has. +_FAILURE_RATES_FILE = "failure_rates.json" + # Convention-dependent analyses: (stem, callable). Each is emitted once per # scoring convention as ``__.json`` (content-based naming: # the filename states both the test and the convention, so a file is never @@ -105,6 +114,12 @@ "tradeoff_correctness_efficiency.json + effect_sizes__intent_to_treat.json", "Correctness vs. efficiency trade-off across strategies.", ), + ( + "reliability", + "failure_rates.json + survivor_bias.json", + "Cross-cutting: how often does a strategy produce nothing usable, " + "from what cause, and how much does the valid-only view flatter it?", + ), ( "robustness", "anova_strategy_by_model__intent_to_treat.json + " @@ -393,6 +408,7 @@ def main(argv: list[str] | None = None) -> int: # enforces that at the boundary instead of relying on a no-op commit. with get_readonly_connection(args.db) as conn: df = load_dataframe(conn) + failures = load_failure_dataframe(conn) if df.empty: print( @@ -407,6 +423,10 @@ def main(argv: list[str] | None = None) -> int: results[filename] = payload _write_json(run_dir / filename, payload) + failure_payload = failure_rates(df, failures) + results[_FAILURE_RATES_FILE] = failure_payload + _write_json(run_dir / _FAILURE_RATES_FILE, failure_payload) + # Convention-dependent analyses: one file per (analysis, convention). for stem, fn in _CONVENTION_ANALYSES: for convention in _CONVENTIONS: diff --git a/src/maestro/analysis/failures.py b/src/maestro/analysis/failures.py new file mode 100644 index 0000000..9261629 --- /dev/null +++ b/src/maestro/analysis/failures.py @@ -0,0 +1,188 @@ +""" +Failure-mode classification for runs that produced no valid output. + +The harness separates reliability (did a run produce anything usable) from +accuracy (how good was it). The accuracy side is analysed in detail by +``statistics.py``; without this module the whole reliability side collapses +to a single failure count, which is not enough to say *why* a strategy is +less reliable. + +## What counts as a failure + +Two disjoint shapes, both of which ``RunResult.success`` rejects: + +- an **errored** run: ``error`` is set, and the string is the classification + evidence. +- a **silent-empty** run: ``error`` is None but the diagram is missing or + blank. No error string exists, so these classify as ``EMPTY_OUTPUT`` from + the row shape alone. They are a real category, not a data defect: a + provider can return whitespace without raising. + +## Where the evidence lives + +For a failed *run*, ``run_results.raw_response`` is NULL: the top-level error +path builds its result before any text exists. The failing text is retained +one level down, on the ``sub_results`` row that failed. Classification +therefore reads the run's ``error`` string and, when a caller supplies it, +the failed sub-result's ``raw_response``. That is why ``classify_failure`` +takes the raw text as a separate optional argument rather than digging it +out of the run row: the run row does not have it. + +## Precedence + +Categories are not mutually exclusive in practice: a truncated response is +usually *also* a JSON parse error, because the truncation is what broke the +parse. Rather than multi-label (which makes rates hard to sum and compare), +each failure gets one primary cause under a fixed precedence, most specific +first. ``_RULES`` is ordered, and the first match wins: + +1. Infrastructure causes that preempt any output judgement (rate limit, + timeout, API error, safety block). If the API never returned, nothing + downstream is meaningful. +2. Empty output. A missing response cannot be a parse error. +3. Truncation, checked *before* the parse rules so a cut-off response is + attributed to the token limit that caused it rather than to the parse + error that is merely its symptom. +4. Schema and parse violations, the ordinary "model wrote the wrong thing" + causes. +5. Orchestration errors: the framework itself misbehaved. +6. ``UNKNOWN`` as the explicit fallback, so an unrecognised string is + visible in the breakdown instead of silently joining a real category. + +Adding a provider means checking whether its error prefixes are covered +here; an unmatched prefix shows up as ``UNKNOWN`` rather than being +mis-filed, which is the failure mode this ordering is built to avoid. +""" + +from __future__ import annotations + +import re +from enum import StrEnum + +# Heuristic ceiling for calling a response truncated. Applied only to text +# that also failed to parse: a long *valid* response is not truncated, and a +# short unparseable one is malformed rather than cut off. The value is a +# judgement call, so it is named rather than inlined at the comparison. +_TRUNCATION_MIN_CHARS = 200 + + +class FailureCause(StrEnum): + """ + Primary cause of one invalid generation. + + A ``StrEnum`` so the value serialises to a plain string in the JSON + breakdown and compares cleanly against DataFrame columns, matching how + ``Strategy`` and ``Tier`` are already handled in ``schemas.py``. + """ + + RATE_LIMIT = "rate_limit" + TIMEOUT = "timeout" + API_ERROR = "api_error" + SAFETY_BLOCK = "safety_block" + EMPTY_OUTPUT = "empty_output" + TRUNCATION = "truncation" + PARSE_ERROR = "parse_error" + SCHEMA_VIOLATION = "schema_violation" + ORCHESTRATION_ERROR = "orchestration_error" + UNKNOWN = "unknown" + + +# Ordered (cause, pattern) rules. Order *is* the precedence documented in the +# module docstring: the first match wins, so never reorder without re-reading +# it. Patterns match case-insensitively against the error string, and are +# anchored on the literal prefixes the providers and strategies emit +# (``RateLimitError:``, ``EmptyResponse:``, ``invalid JSON:``, ...) rather +# than on loose keywords, so an unrelated message that merely mentions +# "timeout" in prose does not get mis-filed. +_RULES: tuple[tuple[FailureCause, re.Pattern[str]], ...] = ( + # 1. Infrastructure. These preempt everything: no usable output existed. + (FailureCause.RATE_LIMIT, re.compile(r"\bRateLimitError\b")), + (FailureCause.TIMEOUT, re.compile(r"\b(?:APITimeoutError|TimeoutError)\b")), + (FailureCause.SAFETY_BLOCK, re.compile(r"\b(?:BlockedResponse|ContentFilter)\b")), + # 2. Empty output, before the parse rules: nothing to parse. + ( + FailureCause.EMPTY_OUTPUT, + re.compile(r"\bEmptyResponse\b|\bempty output from provider\b"), + ), + # CrewAI surfaces an empty LLM reply as its own kickoff message rather + # than an EmptyResponse; it is the same underlying cause. + ( + FailureCause.EMPTY_OUTPUT, + re.compile(r"Invalid response from LLM call\s*-\s*None or empty"), + ), + # 3. Schema violations. Checked before the generic parse rule because the + # structural Mermaid checks (empty label bracket, unbalanced subgraph) + # describe well-formed text that violates the output contract, which is a + # different failure from text that is not parseable at all. + ( + FailureCause.SCHEMA_VIOLATION, + re.compile(r"empty node label bracket|unbalanced subgraph/end"), + ), + # 4. Parse errors: the model did not produce the requested format. + (FailureCause.PARSE_ERROR, re.compile(r"\binvalid JSON\b|\bJSONDecodeError\b")), + # 5. Orchestration: the framework misbehaved, not the model output. + ( + FailureCause.ORCHESTRATION_ERROR, + re.compile(r"Single-call invariant violated|kickoff raised"), + ), + # Generic API error last among the infrastructure family: APIError is the + # SDKs' catch-all base class, so a more specific subclass above must win. + (FailureCause.API_ERROR, re.compile(r"\bAPIError\b")), +) + +# Signatures of a response cut off mid-token. An unterminated string or a +# structure that simply stops is what truncation looks like after the fact; +# the provider does not tell us the token limit was hit. +_TRUNCATION_PATTERN = re.compile( + r"Unterminated string|Expecting value: line \d+ column \d+ \(char \d+\)" +) + + +def classify_failure( + error: str | None, + raw_response: str | None = None, +) -> FailureCause: + """ + Resolve one failed run to its single primary cause. + + ``error`` is the run's error string; ``raw_response`` is the failing text + from the sub-result that produced it, when available (see the module + docstring on why it is a separate argument). The raw text is used only to + separate truncation from an ordinary parse error, which the error string + alone cannot distinguish. + + A ``None``/blank ``error`` classifies as ``EMPTY_OUTPUT``: that is the + silent-empty shape, where a run has no error but no diagram either. + Callers must only pass runs that actually failed, since a *successful* + run also has ``error is None`` and would classify the same way. + """ + if error is None or not error.strip(): + return FailureCause.EMPTY_OUTPUT + + for cause, pattern in _RULES: + if pattern.search(error): + # Truncation masquerades as a parse error: same message, different + # cause. Promote it only with corroborating evidence, a long raw + # response that stops mid-structure, so an ordinary malformed + # short reply is not relabelled. + if cause is FailureCause.PARSE_ERROR and _looks_truncated( + error, raw_response + ): + return FailureCause.TRUNCATION + return cause + + return FailureCause.UNKNOWN + + +def _looks_truncated(error: str, raw_response: str | None) -> bool: + """ + Whether a parse failure is better explained by truncation. + + Requires both a truncation-shaped parser message and a raw response long + enough that running out of tokens is plausible. Without the raw text we + cannot tell the two apart, so we stay conservative and keep the parse-error + label: under-reporting truncation is safer than inventing it. + """ + if raw_response is None or len(raw_response) < _TRUNCATION_MIN_CHARS: + return False + return bool(_TRUNCATION_PATTERN.search(error)) diff --git a/src/maestro/analysis/statistics.py b/src/maestro/analysis/statistics.py index 2ac654e..06d3fdd 100644 --- a/src/maestro/analysis/statistics.py +++ b/src/maestro/analysis/statistics.py @@ -54,7 +54,12 @@ ANOVA conclusion survives a model that accounts for the crossed grouping structure directly instead of by pre-averaging. - **Error taxonomy**: descriptive characterization of the eight taxonomy - counts per strategy (exploratory; no inferential test). + counts per strategy (exploratory; no inferential test). This scores the + *content* of a diagram that was produced; the failure analysis below covers + runs that produced nothing scorable at all. The two are disjoint populations. +- **Failure modes**: per-strategy/model/tier failure rates broken down by + primary cause (see ``failures.py``), plus a survivor-bias number saying how + much the valid-only view flatters each strategy. - **Correctness/efficiency trade-off**: per-strategy correctness against cost and latency, plus a correctness-to-cost ratio. @@ -83,7 +88,8 @@ import sqlite3 from typing import TYPE_CHECKING, Any, Literal -from maestro.db.queries import fetch_analysis_rows +from maestro.analysis.failures import FailureCause, classify_failure +from maestro.db.queries import fetch_analysis_rows, fetch_failure_rows from maestro.experiment_config import CONTROL_STRATEGIES from maestro.schemas import Strategy @@ -892,6 +898,182 @@ def error_taxonomy_by_strategy(df: "pd.DataFrame") -> dict[str, Any]: return out +# --------------------------------------------------------------------------- +# Failure modes +# --------------------------------------------------------------------------- + + +def load_failure_dataframe(conn: sqlite3.Connection) -> "pd.DataFrame": + """ + Load every invalid run, classified by primary failure cause. Read-only. + + Adds a ``failure_cause`` column via ``classify_failure``. The frame is a + different population from ``load_dataframe``'s: one row per *failed* run + rather than per completed run, so the two are never concatenated. Failure + *rates* need both, which is why ``failure_rates`` takes the two frames. + + Returns an empty DataFrame when nothing failed, which is a legitimate + result (a perfect run), not an error. + """ + import pandas as pd + + rows = [dict(r) for r in fetch_failure_rows(conn)] + if not rows: + return pd.DataFrame() + df = pd.DataFrame(rows) + df["failure_cause"] = [ + classify_failure(r["error"], r["failing_raw_response"]).value for r in rows + ] + return df + + +def failure_rates(df: "pd.DataFrame", failures: "pd.DataFrame") -> dict[str, Any]: + """ + Failure rate and cause breakdown per strategy, model, and tier, mirroring + the grouping of the accuracy tables so the reliability and accuracy sides + of a strategy can be read against each other. + + Rates are **pooled counts**, not means of per-cell rates: the denominator + is every run attempted in the group. A per-cell mean would weight a cell + with one run as heavily as a cell with five, which for a rate is the wrong + grain. This deliberately differs from the F1 path, which aggregates per + cell because it is averaging a score rather than counting events. + + Controls are excluded: they never call a model, so their failure rate is + zero by construction and would dilute every pooled denominator it entered. + """ + exp = _experimental(df) + out: dict[str, Any] = { + "schema_version": SCHEMA_VERSION, + "analysis": "failure_rates", + "grouping": ["strategy", "model", "tier"], + "statistic": "pooled_failure_rate", + "causes": [c.value for c in FailureCause], + "excludes_controls": True, + "overall": {}, + "by_strategy": [], + "by_model": [], + "by_tier": [], + } + if exp.empty: + out["status"] = "empty" + return out + + exp_failures = _experimental(failures) if not failures.empty else failures + out["overall"] = _failure_summary(exp, exp_failures) + for field in ("strategy", "model", "tier"): + out[f"by_{field}"] = _failure_breakdown(exp, exp_failures, field) + out["status"] = "ok" + return out + + +def _failure_breakdown( + runs: "pd.DataFrame", failures: "pd.DataFrame", field: str +) -> list[dict[str, Any]]: + """One summary per observed level of ``field``, ordered by level.""" + entries: list[dict[str, Any]] = [] + for level, group in runs.groupby(field, dropna=False): + matching = ( + failures[failures[field] == level] if not failures.empty else failures + ) + entries.append({field: _to_native(level), **_failure_summary(group, matching)}) + return entries + + +def _failure_summary(runs: "pd.DataFrame", failures: "pd.DataFrame") -> dict[str, Any]: + """ + Counts, pooled rate, and per-cause counts for one group. + + ``causes`` carries every ``FailureCause`` member, including the zeros: a + cause absent from the output would be ambiguous between "never happened" + and "not measured", and the zeros are what make two groups comparable + column by column. + """ + n_runs = int(len(runs)) + n_failed = int(len(failures)) + counts = ( + failures["failure_cause"].value_counts().to_dict() if not failures.empty else {} + ) + return { + "n_runs": n_runs, + "n_failed": n_failed, + "failure_rate": (n_failed / n_runs) if n_runs else None, + "causes": {c.value: int(counts.get(c.value, 0)) for c in FailureCause}, + } + + +def survivor_bias( + df: "pd.DataFrame", convention: ScoringConvention = VALID_ONLY +) -> dict[str, Any]: + """ + Quantify survivor bias: does the surviving subset differ systematically + from the full set? + + Reports the primary DV under ``valid_only`` (survivors only) against + ``intent_to_treat`` (every run, failures scored 0.0), per strategy. The + gap between them *is* the bias: a strategy that fails often looks better + under valid_only, because its failures were dropped rather than scored, + and the difference measures exactly how much that dropping flatters it. + + Reported as a number per strategy rather than a single test statistic, + because the bias is not uniform: it scales with each strategy's failure + rate, so one pooled figure would hide the comparison that matters. + + The default convention names the survivor side explicitly; passing + ``INTENT_TO_TREAT`` is accepted but yields a zero gap by construction, + so it exists only for symmetry with the other convention-taking + functions rather than as a useful call. + """ + exp = _experimental(df) + out: dict[str, Any] = { + "schema_version": SCHEMA_VERSION, + "analysis": "survivor_bias", + "dependent_variable": PRIMARY_DV, + "survivor_convention": convention, + "reference_convention": INTENT_TO_TREAT, + "excludes_controls": True, + "by_strategy": [], + } + if exp.empty: + out["status"] = "empty" + return out + + all_runs = aggregate_experimental(df, INTENT_TO_TREAT) + survivors = aggregate_experimental(df, convention) + if all_runs.empty: + out["status"] = "empty" + return out + + for strategy, group in all_runs.groupby("strategy"): + kept = ( + survivors[survivors["strategy"] == strategy] + if not survivors.empty + else survivors + ) + all_mean = _to_native(group[PRIMARY_DV].mean()) + kept_mean = _to_native(kept[PRIMARY_DV].mean()) if not kept.empty else None + gap = ( + kept_mean - all_mean + if kept_mean is not None and all_mean is not None + else None + ) + out["by_strategy"].append( + { + "strategy": strategy, + "n_cells_all": int(len(group)), + "n_cells_survivors": int(len(kept)), + # Cells lost entirely: every run in them failed, so valid_only + # has nothing to average and the cell vanishes from the frame. + "n_cells_dropped": int(len(group)) - int(len(kept)), + "mean_all_runs": all_mean, + "mean_survivors": kept_mean, + "survivor_bias": gap, + } + ) + out["status"] = "ok" + return out + + # --------------------------------------------------------------------------- # Correctness / efficiency trade-off # --------------------------------------------------------------------------- diff --git a/src/maestro/db/queries.py b/src/maestro/db/queries.py index ef417a5..3492813 100644 --- a/src/maestro/db/queries.py +++ b/src/maestro/db/queries.py @@ -232,6 +232,56 @@ def fetch_all_results(conn: sqlite3.Connection) -> list[sqlite3.Row]: ).fetchall() +def fetch_failure_rows(conn: sqlite3.Connection) -> list[sqlite3.Row]: + """ + One row per *invalid* run, for failure-mode classification. Read-only. + + Covers both failure shapes ``RunResult.success`` rejects: an errored run, + and a run with no error but a missing or blank diagram. The WHERE clause + is the negation of the success condition used by ``fetch_completed_cells``, + so the two partition the run set and a row can never be counted as both. + + ``failing_raw_response`` comes from a correlated subquery over + ``sub_results`` rather than from ``run_results``: on a failed run the + top-level ``raw_response`` is always NULL, because the error result is + built before any text exists. The failing text is retained one level down, + on the sub-result whose step failed. The subquery takes the *first* failed + step by ``step_number``, which is the step that actually broke the run + (later steps never ran). It stays NULL when the framework produced no text + at all, which is itself classifiable evidence. + + Left-joining ``sub_results`` instead would multiply a run into one row per + sub-result and inflate every failure count; the subquery keeps the grain at + one row per run. + """ + return conn.execute( + """ + SELECT + c.run_id AS run_id, + c.strategy AS strategy, + c.model AS model, + c.example_id AS example_id, + c.tier AS tier, + c.run_number AS run_number, + r.error AS error, + ( + SELECT s.raw_response + FROM sub_results s + WHERE s.run_id = c.run_id + AND s.error IS NOT NULL + ORDER BY s.step_number + LIMIT 1 + ) AS failing_raw_response + FROM run_configs c + JOIN run_results r ON c.run_id = r.run_id + WHERE r.error IS NOT NULL + OR r.output_diagram_code IS NULL + OR TRIM(r.output_diagram_code) = '' + ORDER BY c.timestamp + """, + ).fetchall() + + def fetch_analysis_rows(conn: sqlite3.Connection) -> list[sqlite3.Row]: """ Three-way join (run_configs ⋈ run_results, then LEFT ⋈ metric_results) diff --git a/tests/analysis/test_failures.py b/tests/analysis/test_failures.py new file mode 100644 index 0000000..cd94561 --- /dev/null +++ b/tests/analysis/test_failures.py @@ -0,0 +1,446 @@ +""" +Tests for failure-mode classification (src/maestro/analysis/failures.py) and +the failure analyses in statistics.py. + +Two layers, deliberately separated: + + 1. ``classify_failure`` is pure, so it is tested directly against the error + strings the codebase actually emits. The strings are copied from the + provider/strategy error paths and from the production database, not + invented, so a reworded error message fails a test instead of silently + becoming ``UNKNOWN`` in a live run. + 2. The rate and survivor-bias functions are exercised through the *real* + schema (db.client SCHEMA + db.queries inserts), matching the approach in + test_statistics.py: the correlated sub_results subquery in + fetch_failure_rows is exactly the part worth testing against real SQL. +""" + +from __future__ import annotations + +import sqlite3 +import uuid + +import pytest + +pytest.importorskip("pandas") + +from maestro.analysis.failures import ( # noqa: E402 + FailureCause, + classify_failure, +) +from maestro.analysis.statistics import ( # noqa: E402 + failure_rates, + load_dataframe, + load_failure_dataframe, + survivor_bias, +) +from maestro.db.client import SCHEMA # noqa: E402 +from maestro.db.queries import ( # noqa: E402 + fetch_failure_rows, + insert_run_config, + insert_run_result, + insert_sub_result, +) +from maestro.schemas import ( # noqa: E402 + RunConfig, + RunResult, + Strategy, + SubResult, + Tier, +) + + +def _conn() -> sqlite3.Connection: + conn = sqlite3.connect(":memory:") + conn.row_factory = sqlite3.Row + conn.executescript(SCHEMA) + return conn + + +# --------------------------------------------------------------------------- +# classify_failure +# --------------------------------------------------------------------------- + +# Error strings taken verbatim from the provider error paths and from the +# production database, paired with the cause each must resolve to. +_REAL_ERRORS = [ + ( + "Step 3 (generate_mermaid) failed: Invalid generate_mermaid output on " + 'attempt 2: empty node label bracket (e.g. node_id[""])', + FailureCause.SCHEMA_VIOLATION, + ), + ( + "Step 3 (generate_mermaid) failed: Invalid generate_mermaid output on " + "attempt 2: unbalanced subgraph/end (6 subgraph, 5 end)", + FailureCause.SCHEMA_VIOLATION, + ), + ( + "Step 1 (extract_entities) failed: Invalid extract_entities output on " + "attempt 2: invalid JSON: Expecting property name enclosed in double " + "quotes: line 26 column 5 (char 500)", + FailureCause.PARSE_ERROR, + ), + ( + "Step 3 (generate_mermaid) failed: empty output from provider", + FailureCause.EMPTY_OUTPUT, + ), + ( + "Step 3 (generate_mermaid) failed: CrewAI kickoff raised on attempt 2: " + "Invalid response from LLM call - None or empty.", + FailureCause.EMPTY_OUTPUT, + ), + ( + "Step 3 (generate_mermaid) failed: Single-call invariant violated on " + "attempt 2: expected 1 new call, got 3", + FailureCause.ORCHESTRATION_ERROR, + ), + ("RateLimitError: 429 rate limit exceeded", FailureCause.RATE_LIMIT), + ("TimeoutError: request timed out", FailureCause.TIMEOUT), + ("APITimeoutError: deadline exceeded", FailureCause.TIMEOUT), + ("APIError: 500 internal server error", FailureCause.API_ERROR), + ("BlockedResponse: response blocked by safety filter", FailureCause.SAFETY_BLOCK), + ("EmptyResponse: openai returned no content", FailureCause.EMPTY_OUTPUT), + ("EmptyResponse: anthropic returned no text content", FailureCause.EMPTY_OUTPUT), +] + + +@pytest.mark.parametrize("error,expected", _REAL_ERRORS) +def test_classifies_real_error_strings(error: str, expected: FailureCause) -> None: + assert classify_failure(error) is expected + + +def test_no_error_is_empty_output() -> None: + """The silent-empty shape: no error string, but no diagram either.""" + assert classify_failure(None) is FailureCause.EMPTY_OUTPUT + assert classify_failure(" ") is FailureCause.EMPTY_OUTPUT + + +def test_unrecognized_error_is_unknown_not_misfiled() -> None: + """ + An unmatched string must surface as UNKNOWN. Silently absorbing it into a + real category is the failure mode the ordered rules exist to prevent. + """ + assert classify_failure("KrakenError: the kraken woke up") is FailureCause.UNKNOWN + + +def test_specific_api_error_beats_generic_api_error() -> None: + """ + APIError is the SDKs' catch-all base class, so a message naming a more + specific subclass must not be filed under the generic cause. + """ + assert classify_failure("RateLimitError: 429") is FailureCause.RATE_LIMIT + assert classify_failure("APITimeoutError: slow") is FailureCause.TIMEOUT + + +def test_truncation_promoted_over_parse_error_with_long_raw() -> None: + """ + A long response that stops mid-string is truncation, not a parse error: + the parse failure is the symptom, the token limit is the cause. + """ + error = ( + "Step 1 (extract_entities) failed: Invalid extract_entities output on " + "attempt 2: invalid JSON: Unterminated string starting at: line 194 " + "column 15 (char 4400)" + ) + assert classify_failure(error, "x" * 900) is FailureCause.TRUNCATION + + +def test_truncation_not_promoted_without_corroborating_raw() -> None: + """ + Without a long raw response there is no evidence of truncation, so the + conservative parse-error label stands. Under-reporting truncation is + safer than inventing it. + """ + error = ( + "Invalid extract_entities output on attempt 2: invalid JSON: " + "Unterminated string starting at: line 4 column 1 (char 40)" + ) + assert classify_failure(error, "short") is FailureCause.PARSE_ERROR + assert classify_failure(error, None) is FailureCause.PARSE_ERROR + + +def test_empty_output_precedes_parse_rules() -> None: + """Nothing came back, so there was nothing to parse.""" + assert ( + classify_failure("EmptyResponse: no content", "x" * 900) + is FailureCause.EMPTY_OUTPUT + ) + + +# --------------------------------------------------------------------------- +# fetch_failure_rows / failure_rates +# --------------------------------------------------------------------------- + + +def _insert_run( + conn: sqlite3.Connection, + *, + strategy: Strategy, + error: str | None, + diagram: str | None = "graph TD; a-->b", + sub_errors: list[tuple[int, str | None, str | None]] | None = None, + model: str = "model-a", + tier: Tier = Tier.SIMPLE, + run_number: int = 1, +) -> uuid.UUID: + """ + Insert one config+result, plus optional sub_results. + + ``sub_errors`` entries are (step_number, error, raw_response), letting a + test build the multi-step shape where the failing text lives on the + sub-result rather than the run row. + """ + run_id = uuid.uuid4() + insert_run_config( + conn, + RunConfig( + run_id=run_id, + strategy=strategy, + model=model, + example_id="ex_01", + tier=tier, + run_number=run_number, + ), + ) + insert_run_result( + conn, + RunResult( + run_id=run_id, + output_diagram_code=diagram, + prompt_tokens=10, + completion_tokens=10, + duration_ms=100, + cost_usd=0.001, + error=error, + ), + ) + for step, sub_error, raw in sub_errors or []: + insert_sub_result( + conn, + SubResult( + run_id=run_id, + step_number=step, + step_name=f"step_{step}", + output_text=None if sub_error else "ok", + raw_response=raw, + prompt_tokens=5, + completion_tokens=5, + duration_ms=50, + cost_usd=0.0005, + error=sub_error, + ), + ) + return run_id + + +def test_fetch_failure_rows_covers_both_failure_shapes() -> None: + """ + An errored run and a no-error-but-blank-diagram run are both failures; + a successful run is neither. + """ + conn = _conn() + _insert_run(conn, strategy=Strategy.SOP_BASED, error="APIError: 500") + _insert_run(conn, strategy=Strategy.SINGLE_AGENT, error=None, diagram=" ") + _insert_run(conn, strategy=Strategy.SINGLE_AGENT, error=None, diagram=None) + _insert_run(conn, strategy=Strategy.SINGLE_AGENT, error=None) + + rows = fetch_failure_rows(conn) + assert len(rows) == 3 + + +def test_failing_raw_response_comes_from_first_failed_step() -> None: + """ + The raw text is pulled from the earliest failed sub-result: that is the + step that broke the run, and later steps never ran. + """ + conn = _conn() + _insert_run( + conn, + strategy=Strategy.SOP_BASED, + error="Step 1 (extract_entities) failed: invalid JSON: Expecting value", + diagram=None, + sub_errors=[ + (1, "invalid JSON: Expecting value", "the raw step-1 text"), + (2, "downstream failure", "later text"), + ], + ) + rows = fetch_failure_rows(conn) + assert len(rows) == 1 + assert rows[0]["failing_raw_response"] == "the raw step-1 text" + + +def test_one_row_per_run_despite_many_sub_results() -> None: + """ + The correlated subquery must not multiply a run into one row per + sub-result, which would inflate every failure count. + """ + conn = _conn() + _insert_run( + conn, + strategy=Strategy.CREW_AI, + error="Step 1 failed: invalid JSON: Expecting value", + diagram=None, + sub_errors=[ + (i, "invalid JSON: Expecting value", f"raw {i}") for i in range(1, 6) + ], + ) + assert len(fetch_failure_rows(conn)) == 1 + + +def test_failure_rates_are_pooled_counts() -> None: + """ + Rate is failures over runs attempted in the group, not a mean of per-cell + rates: 1 failure in 4 runs is 0.25 regardless of how the runs split across + cells. + """ + conn = _conn() + for i in range(3): + _insert_run(conn, strategy=Strategy.SOP_BASED, error=None, run_number=i + 1) + _insert_run( + conn, + strategy=Strategy.SOP_BASED, + error="APIError: 500", + diagram=None, + run_number=4, + ) + + payload = failure_rates(load_dataframe(conn), load_failure_dataframe(conn)) + assert payload["status"] == "ok" + assert payload["overall"]["n_runs"] == 4 + assert payload["overall"]["n_failed"] == 1 + assert payload["overall"]["failure_rate"] == pytest.approx(0.25) + assert payload["overall"]["causes"]["api_error"] == 1 + + +def test_failure_rates_report_every_cause_including_zeros() -> None: + """ + A cause absent from the output would be ambiguous between "never happened" + and "not measured"; the zeros are what make groups comparable. + """ + conn = _conn() + _insert_run(conn, strategy=Strategy.SOP_BASED, error="APIError: 500", diagram=None) + payload = failure_rates(load_dataframe(conn), load_failure_dataframe(conn)) + assert set(payload["overall"]["causes"]) == {c.value for c in FailureCause} + assert payload["overall"]["causes"]["rate_limit"] == 0 + + +def test_failure_rates_exclude_controls() -> None: + """Controls never call a model, so they must not dilute the denominator.""" + conn = _conn() + _insert_run(conn, strategy=Strategy.SOP_BASED, error="APIError: 500", diagram=None) + for i in range(5): + _insert_run(conn, strategy=Strategy.NULL_CONTROL, error=None, run_number=i + 1) + + payload = failure_rates(load_dataframe(conn), load_failure_dataframe(conn)) + assert payload["overall"]["n_runs"] == 1 + assert payload["overall"]["failure_rate"] == pytest.approx(1.0) + strategies = {row["strategy"] for row in payload["by_strategy"]} + assert Strategy.NULL_CONTROL.value not in strategies + + +def test_failure_rates_empty_db_is_status_empty_not_crash() -> None: + conn = _conn() + payload = failure_rates(load_dataframe(conn), load_failure_dataframe(conn)) + assert payload["status"] == "empty" + + +def test_load_failure_dataframe_empty_when_nothing_failed() -> None: + """A perfect run is a legitimate result, not an error.""" + conn = _conn() + _insert_run(conn, strategy=Strategy.SOP_BASED, error=None) + assert load_failure_dataframe(conn).empty + + +# --------------------------------------------------------------------------- +# survivor_bias +# --------------------------------------------------------------------------- + + +def test_survivor_bias_is_zero_without_failures() -> None: + """ + With nothing dropped, the two conventions see the same runs, so the gap + must be exactly zero rather than merely small. + """ + pytest.importorskip("statsmodels") + conn = _conn() + for i in range(3): + _insert_run_with_metric(conn, f1=0.8, run_number=i + 1) + + payload = survivor_bias(load_dataframe(conn)) + assert payload["status"] == "ok" + row = payload["by_strategy"][0] + assert row["survivor_bias"] == pytest.approx(0.0) + assert row["n_cells_dropped"] == 0 + + +def test_survivor_bias_positive_when_failures_dropped() -> None: + """ + Dropping a failed run raises the surviving mean above the all-runs mean: + that gap is precisely how much valid_only flatters the strategy. + """ + pytest.importorskip("statsmodels") + conn = _conn() + _insert_run_with_metric(conn, f1=0.9, run_number=1) + # A failed run: no metric row, so intent_to_treat scores it 0.0 and + # valid_only drops it entirely. + _insert_run( + conn, + strategy=Strategy.SOP_BASED, + error="APIError: 500", + diagram=None, + run_number=2, + ) + + payload = survivor_bias(load_dataframe(conn)) + row = payload["by_strategy"][0] + assert row["mean_all_runs"] == pytest.approx(0.45) # (0.9 + 0.0) / 2 + assert row["mean_survivors"] == pytest.approx(0.9) + assert row["survivor_bias"] == pytest.approx(0.45) + + +def _insert_run_with_metric( + conn: sqlite3.Connection, + *, + f1: float, + run_number: int, + strategy: Strategy = Strategy.SOP_BASED, +) -> None: + """Insert a successful run carrying a metric row with the given F1.""" + from maestro.db.queries import insert_metric_result + from maestro.schemas import MetricResult + + run_id = _insert_run(conn, strategy=strategy, error=None, run_number=run_number) + insert_metric_result( + conn, + MetricResult( + run_id=run_id, + parses_valid=True, + entity_id_precision=f1, + entity_id_recall=f1, + entity_id_f1=f1, + entity_name_precision=0.0, + entity_name_recall=0.0, + entity_name_f1=0.0, + entity_lemma_precision=0.0, + entity_lemma_recall=0.0, + entity_lemma_f1=0.0, + relationship_relaxed_precision=0.0, + relationship_relaxed_recall=0.0, + relationship_relaxed_f1=0.0, + relationship_strict_precision=0.0, + relationship_strict_recall=0.0, + relationship_strict_f1=0.0, + entities_in_output=0, + entities_in_truth=0, + relationships_in_output=0, + relationships_in_truth=0, + missing_entities=0, + extra_entities=0, + false_entities=0, + duplicate_entities=0, + missing_relationships=0, + extra_relationships=0, + false_relationships=0, + duplicate_relationships=0, + ), + ) From a66e4fe0c48d3efb228732da0cfecd64d1b00e2c Mon Sep 17 00:00:00 2001 From: Colinho22 <48288595+Colinho22@users.noreply.github.com> Date: Tue, 15 Sep 2026 20:13:52 +0200 Subject: [PATCH 2/3] fix: match failure-classification patterns case-insensitively The rules comment claimed case-insensitive matching but no pattern carried re.IGNORECASE, so a case variant fell through to UNKNOWN. Error text comes from vendor SDKs and third-party frameworks, so its casing is not ours to rely on: a provider rewording APIError to ApiError would have pushed a whole category into UNKNOWN silently, which is the mis-filing the ordered rules exist to prevent. Compilation now routes through a _rule helper that applies the flag in one place, so a new rule cannot forget it. No reclassification: all 478 failures in the existing corpus keep their cause and the counts are unchanged, since the real strings already matched on exact case. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013jLSXwVs1RmfUbCUbQurCJ --- src/maestro/analysis/failures.py | 45 ++++++++++++++++++++------------ tests/analysis/test_failures.py | 27 +++++++++++++++++++ 2 files changed, 56 insertions(+), 16 deletions(-) diff --git a/src/maestro/analysis/failures.py b/src/maestro/analysis/failures.py index 9261629..568f389 100644 --- a/src/maestro/analysis/failures.py +++ b/src/maestro/analysis/failures.py @@ -87,28 +87,40 @@ class FailureCause(StrEnum): UNKNOWN = "unknown" +def _rule(pattern: str) -> re.Pattern[str]: + """ + Compile one classification pattern. + + Every pattern is case-insensitive: the error text comes from vendor SDKs + and third-party frameworks, so its casing is not ours to rely on. A + provider rewording ``APIError`` to ``ApiError`` would otherwise push a + whole category into ``UNKNOWN`` silently. Centralised here so a new rule + cannot forget the flag. + """ + return re.compile(pattern, re.IGNORECASE) + + # Ordered (cause, pattern) rules. Order *is* the precedence documented in the # module docstring: the first match wins, so never reorder without re-reading -# it. Patterns match case-insensitively against the error string, and are -# anchored on the literal prefixes the providers and strategies emit -# (``RateLimitError:``, ``EmptyResponse:``, ``invalid JSON:``, ...) rather -# than on loose keywords, so an unrelated message that merely mentions -# "timeout" in prose does not get mis-filed. +# it. Patterns are anchored on the literal prefixes the providers and +# strategies emit (``RateLimitError:``, ``EmptyResponse:``, ``invalid JSON:``, +# ...) rather than on loose keywords, so an unrelated message that merely +# mentions "timeout" in prose does not get mis-filed. _RULES: tuple[tuple[FailureCause, re.Pattern[str]], ...] = ( # 1. Infrastructure. These preempt everything: no usable output existed. - (FailureCause.RATE_LIMIT, re.compile(r"\bRateLimitError\b")), - (FailureCause.TIMEOUT, re.compile(r"\b(?:APITimeoutError|TimeoutError)\b")), - (FailureCause.SAFETY_BLOCK, re.compile(r"\b(?:BlockedResponse|ContentFilter)\b")), + (FailureCause.RATE_LIMIT, _rule(r"\bRateLimitError\b")), + (FailureCause.TIMEOUT, _rule(r"\b(?:APITimeoutError|TimeoutError)\b")), + (FailureCause.SAFETY_BLOCK, _rule(r"\b(?:BlockedResponse|ContentFilter)\b")), # 2. Empty output, before the parse rules: nothing to parse. ( FailureCause.EMPTY_OUTPUT, - re.compile(r"\bEmptyResponse\b|\bempty output from provider\b"), + _rule(r"\bEmptyResponse\b|\bempty output from provider\b"), ), # CrewAI surfaces an empty LLM reply as its own kickoff message rather # than an EmptyResponse; it is the same underlying cause. ( FailureCause.EMPTY_OUTPUT, - re.compile(r"Invalid response from LLM call\s*-\s*None or empty"), + _rule(r"Invalid response from LLM call\s*-\s*None or empty"), ), # 3. Schema violations. Checked before the generic parse rule because the # structural Mermaid checks (empty label bracket, unbalanced subgraph) @@ -116,24 +128,25 @@ class FailureCause(StrEnum): # different failure from text that is not parseable at all. ( FailureCause.SCHEMA_VIOLATION, - re.compile(r"empty node label bracket|unbalanced subgraph/end"), + _rule(r"empty node label bracket|unbalanced subgraph/end"), ), # 4. Parse errors: the model did not produce the requested format. - (FailureCause.PARSE_ERROR, re.compile(r"\binvalid JSON\b|\bJSONDecodeError\b")), + (FailureCause.PARSE_ERROR, _rule(r"\binvalid JSON\b|\bJSONDecodeError\b")), # 5. Orchestration: the framework misbehaved, not the model output. ( FailureCause.ORCHESTRATION_ERROR, - re.compile(r"Single-call invariant violated|kickoff raised"), + _rule(r"Single-call invariant violated|kickoff raised"), ), # Generic API error last among the infrastructure family: APIError is the # SDKs' catch-all base class, so a more specific subclass above must win. - (FailureCause.API_ERROR, re.compile(r"\bAPIError\b")), + (FailureCause.API_ERROR, _rule(r"\bAPIError\b")), ) # Signatures of a response cut off mid-token. An unterminated string or a # structure that simply stops is what truncation looks like after the fact; -# the provider does not tell us the token limit was hit. -_TRUNCATION_PATTERN = re.compile( +# the provider does not tell us the token limit was hit. Case-insensitive for +# the same reason as the rules above: the text is the json module's, not ours. +_TRUNCATION_PATTERN = _rule( r"Unterminated string|Expecting value: line \d+ column \d+ \(char \d+\)" ) diff --git a/tests/analysis/test_failures.py b/tests/analysis/test_failures.py index cd94561..76cba82 100644 --- a/tests/analysis/test_failures.py +++ b/tests/analysis/test_failures.py @@ -167,6 +167,33 @@ def test_empty_output_precedes_parse_rules() -> None: ) +@pytest.mark.parametrize( + "error,expected", + [ + ("ratelimiterror: 429", FailureCause.RATE_LIMIT), + ("RATELIMITERROR: 429", FailureCause.RATE_LIMIT), + ("apierror: 500", FailureCause.API_ERROR), + ("ApiError: 500", FailureCause.API_ERROR), + ("emptyresponse: no content", FailureCause.EMPTY_OUTPUT), + ("Invalid Json: bad", FailureCause.PARSE_ERROR), + ("Empty Node Label Bracket", FailureCause.SCHEMA_VIOLATION), + ], +) +def test_classification_is_case_insensitive(error: str, expected: FailureCause) -> None: + """ + Error text originates in vendor SDKs and third-party frameworks, so its + casing is not ours to rely on: a provider rewording ``APIError`` to + ``ApiError`` must not silently push a whole category into UNKNOWN. + """ + assert classify_failure(error) is expected + + +def test_truncation_detection_is_case_insensitive() -> None: + """The truncation signatures come from the json module, not from us.""" + error = "invalid json: unterminated string starting at: line 9 column 2" + assert classify_failure(error, "x" * 900) is FailureCause.TRUNCATION + + # --------------------------------------------------------------------------- # fetch_failure_rows / failure_rates # --------------------------------------------------------------------------- From 85e08f7804db6a6b10468fbf7dd1c04621198e1d Mon Sep 17 00:00:00 2001 From: Colinho22 <48288595+Colinho22@users.noreply.github.com> Date: Tue, 15 Sep 2026 20:28:50 +0200 Subject: [PATCH 3/3] docs: document the empty status in the analysis contract The status contract listed only "ok" and "skipped", but "empty" is emitted wherever an analysis has no experimental rows (describe has done so since before the failure analyses landed). A reader following the documented contract would not know the third value existed. Also fixes the sparse-corpora sample, which indexed payload['reason'] in its else branch: an "empty" payload carries no reason, so the documented pattern raised KeyError on exactly the case it was meant to handle. The distinction is worth keeping explicit: "empty" means no data, while "skipped" means data that will not support this particular test. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013jLSXwVs1RmfUbCUbQurCJ --- docs/analysis.md | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/docs/analysis.md b/docs/analysis.md index 60d8869..e4e2254 100644 --- a/docs/analysis.md +++ b/docs/analysis.md @@ -75,6 +75,10 @@ Every file has a top-level `status` string: A `reason` string explains which factor was underpopulated. Re-runs will populate the analysis automatically once the corpus grows; no code change is needed. +- `"empty"`: there were no experimental rows to analyse at all, so the + analysis had nothing to run on rather than a factor too sparse to fit. + The distinction from `"skipped"` is deliberate: `"empty"` means no data, + `"skipped"` means data that will not support this particular test. ### 3.1 `descriptive.json` @@ -343,8 +347,10 @@ NULL regardless of operating system. The corpus can under-populate a factor (only one input tier, only one model, only two strategies). Analyses that need at least two levels of that factor return `status="skipped"` with a `reason` string that names -the underpopulated factor. Downstream code should check the status -before reading terms: +the underpopulated factor. An analysis with no experimental rows at all +returns `status="empty"` instead, and carries no `reason`: nothing was +too sparse to fit, there was simply nothing to fit. Downstream code +should check the status before reading terms: ```python import json @@ -353,7 +359,8 @@ payload = json.loads(open("output/analysis/.../anova_strategy_by_tier.json").rea if payload["status"] == "ok": interaction_p = payload["terms"]["strategy:tier"]["p"] else: - print(f"skipped: {payload['reason']}") + # "empty" carries no reason, so do not index it unconditionally. + print(f"{payload['status']}: {payload.get('reason', 'no experimental rows')}") ``` Every consumer (the report builder, the dashboard) uses this pattern; a