From 9928da27f3e6d9f845be58a5d1743976ef541a9a Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 18 May 2026 03:48:23 +0000 Subject: [PATCH] =?UTF-8?q?fix(claude=5Fcode):=20bugbot=20=E2=80=94=20aggr?= =?UTF-8?q?egate=20all=20fail=20errors=20+=20structural=20test=20every=20m?= =?UTF-8?q?anifest=20feature?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses three Bugbot concerns flagged on PR #28027 that are real behavioral / coverage gaps: 1. matrix_builder._aggregate_cell now joins every failing tier's error in the published cell instead of silently dropping all but the first. When Haiku 429s and Opus times out on the same cell, both diagnostics land in the matrix JSON so docs-page triage can name both outliers. 2. _aggregate_cell treats 'not_tested' rows as absent data: they're dropped before computing the cell status. Previously a mixed (pass, not_tested) cell silently fell through to 'not_tested', discarding the passing tiers and hiding real coverage from the published matrix. A cell still aggregates to 'not_tested' when *every* row is 'not_tested' (or there are no rows at all). 3. test_v0_layout.py now structurally validates every feature declared in manifest.yaml (directory exists, __init__.py exists, every per-provider test_.py exists), not just the original six v0 rows. The EXPECTED_FEATURE_IDS / EXPECTED_PROVIDERS anchor constants still pin v0 positions; the new manifest-driven tests extend the same structural guarantees to every post-v0 row so a broken directory in 'count_tokens', 'tool_search', 'web_search', etc. fails CI instead of silently becoming a 'not_tested' cell. Three new builder tests pin the new aggregation behavior: - mixed pass + not_tested surfaces as pass - all-not_tested stays not_tested - multiple fail errors are joined with '; ' Co-authored-by: Mateo Wang --- .../test_matrix_builder.py | 99 +++++++++++++++++++ .../_builder_unit_tests/test_v0_layout.py | 48 +++++++++ tests/claude_code/matrix_builder.py | 26 +++-- 3 files changed, 167 insertions(+), 6 deletions(-) diff --git a/tests/claude_code/_builder_unit_tests/test_matrix_builder.py b/tests/claude_code/_builder_unit_tests/test_matrix_builder.py index 17411464050..2644e092e48 100644 --- a/tests/claude_code/_builder_unit_tests/test_matrix_builder.py +++ b/tests/claude_code/_builder_unit_tests/test_matrix_builder.py @@ -90,6 +90,105 @@ def test_build_matrix_any_fail_makes_cell_fail(): assert cell["error"] == "[claude-opus-4-7] timeout" +def test_build_matrix_joins_all_failure_errors_in_one_cell(): + """When multiple tiers fail for different reasons within the same cell, + every failure's error must appear in the published cell so triage + isn't reduced to a single tier's diagnostic. + """ + manifest = { + "schema_version": "1", + "providers": ["anthropic"], + "features": [{"id": "f", "name": "F"}], + } + results = [ + { + "feature_id": "f", + "provider": "anthropic", + "result": {"status": "fail", "error": "[claude-haiku-4-5] 429"}, + }, + {"feature_id": "f", "provider": "anthropic", "result": {"status": "pass"}}, + { + "feature_id": "f", + "provider": "anthropic", + "result": {"status": "fail", "error": "[claude-opus-4-7] timeout"}, + }, + ] + matrix = build_matrix( + manifest=manifest, + results=results, + litellm_version="v", + claude_code_version="c", + generated_at="t", + ) + cell = matrix["features"][0]["providers"]["anthropic"] + assert cell["status"] == "fail" + assert "[claude-haiku-4-5] 429" in cell["error"] + assert "[claude-opus-4-7] timeout" in cell["error"] + + +def test_build_matrix_mixed_pass_and_not_tested_surfaces_pass(): + """A `not_tested` row mixed with `pass` rows must not silently demote + the cell to `not_tested` — `not_tested` is "absent data", not a + negative signal. Otherwise a partial crash mid-test, or a test that + explicitly recorded "tier didn't run", would discard real passing + results from the published cell. + """ + manifest = { + "schema_version": "1", + "providers": ["anthropic"], + "features": [{"id": "f", "name": "F"}], + } + results = [ + {"feature_id": "f", "provider": "anthropic", "result": {"status": "pass"}}, + { + "feature_id": "f", + "provider": "anthropic", + "result": {"status": "not_tested"}, + }, + {"feature_id": "f", "provider": "anthropic", "result": {"status": "pass"}}, + ] + matrix = build_matrix( + manifest=manifest, + results=results, + litellm_version="v", + claude_code_version="c", + generated_at="t", + ) + assert matrix["features"][0]["providers"]["anthropic"] == {"status": "pass"} + + +def test_build_matrix_all_not_tested_stays_not_tested(): + """A cell whose every row is `not_tested` (or empty) must remain + `not_tested` — the absent-data rule only drops `not_tested` rows + when there's other signal to surface. + """ + manifest = { + "schema_version": "1", + "providers": ["anthropic"], + "features": [{"id": "f", "name": "F"}], + } + results = [ + { + "feature_id": "f", + "provider": "anthropic", + "result": {"status": "not_tested"}, + }, + { + "feature_id": "f", + "provider": "anthropic", + "result": {"status": "not_tested"}, + }, + ] + matrix = build_matrix( + manifest=manifest, + results=results, + litellm_version="v", + claude_code_version="c", + generated_at="t", + ) + assert matrix["features"][0]["providers"]["anthropic"] == {"status": "not_tested"} + + def test_build_matrix_fills_not_tested_for_missing_cells(): manifest = { "schema_version": "1", diff --git a/tests/claude_code/_builder_unit_tests/test_v0_layout.py b/tests/claude_code/_builder_unit_tests/test_v0_layout.py index 459db6cb153..b1745008fac 100644 --- a/tests/claude_code/_builder_unit_tests/test_v0_layout.py +++ b/tests/claude_code/_builder_unit_tests/test_v0_layout.py @@ -46,6 +46,23 @@ EXPECTED_PROVIDERS = [ ] +def _all_manifest_feature_ids() -> list[str]: + """Every feature_id currently declared in `manifest.yaml`. + + Evaluated at import time so the result can drive parametrized + structural tests below. Used to catch layout drift on post-v0 + feature rows added after the matrix shipped — the v0 anchor + constants above only validate the original six rows by design. + """ + return [ + feature["id"] + for feature in yaml.safe_load(MANIFEST_PATH.read_text())["features"] + ] + + +ALL_FEATURE_IDS = _all_manifest_feature_ids() + + @pytest.fixture(scope="module") def manifest() -> dict: return yaml.safe_load(MANIFEST_PATH.read_text()) @@ -93,6 +110,37 @@ def test_feature_directory_has_init_file(feature_id): assert init_file.is_file(), f"missing __init__.py: {init_file}" +# Manifest-driven structural tests: every feature in `manifest.yaml` +# (v0 and post-v0 alike) must have the expected on-disk layout. The +# v0-only tests above pin the position of the original six rows; these +# extend the same structural guarantees to any row added afterward so +# a broken post-v0 directory still fails CI. +@pytest.mark.parametrize("feature_id", ALL_FEATURE_IDS) +def test_every_manifest_feature_has_directory(feature_id): + feature_dir = REPO_ROOT / feature_id + assert feature_dir.is_dir(), ( + f"manifest declares {feature_id!r} but {feature_dir} is missing — " + "feature_id MUST match its on-disk directory (see manifest.yaml header)." + ) + + +@pytest.mark.parametrize("feature_id", ALL_FEATURE_IDS) +def test_every_manifest_feature_has_init_file(feature_id): + init_file = REPO_ROOT / feature_id / "__init__.py" + assert init_file.is_file(), f"missing __init__.py: {init_file}" + + +@pytest.mark.parametrize("feature_id", ALL_FEATURE_IDS) +@pytest.mark.parametrize("provider", EXPECTED_PROVIDERS) +def test_every_manifest_feature_has_per_provider_test_file(feature_id, provider): + """Every (feature, provider) cell in the rendered matrix must be + backed by a per-provider test file. Without this check, a missing + file silently becomes a `not_tested` cell in the published matrix + rather than a CI failure surfacing the layout drift.""" + test_file = REPO_ROOT / feature_id / f"test_{provider}.py" + assert test_file.is_file(), f"missing per-provider test file: {test_file}" + + @pytest.mark.parametrize("feature_id", EXPECTED_FEATURE_IDS) @pytest.mark.parametrize("provider", EXPECTED_PROVIDERS) def test_per_provider_test_file_imports_and_parametrizes_three_models( diff --git a/tests/claude_code/matrix_builder.py b/tests/claude_code/matrix_builder.py index b9bf8ecc598..8555805dab2 100644 --- a/tests/claude_code/matrix_builder.py +++ b/tests/claude_code/matrix_builder.py @@ -130,26 +130,40 @@ def _aggregate_cell(results: Sequence[Mapping[str, Any]]) -> Dict[str, Any]: """Aggregate a list of per-model results into a single cell status. Order of precedence (most informative wins): - - Any `fail` → cell is `fail` with the first failure's error. + - Any `fail` → cell is `fail` with every failing model's error + joined by `"; "` so a multi-tier breakage doesn't silently hide + all but the first error from the published matrix. - `not_applicable` → cell is `not_applicable` with the reason. - `pass` → cell is `pass`. - empty / nothing recognized → `not_tested`. + + `not_tested` rows are treated as absent data: they're dropped before + aggregation so a mix of (pass, not_tested) — e.g. from a partial + crash or a test that explicitly recorded "this tier didn't run" — + still surfaces the passing tiers rather than silently demoting the + whole cell to `not_tested`. A cell is only `not_tested` when *every* + row is `not_tested` (or there are no rows at all). """ if not results: return {"status": "not_tested"} - for r in results: - if r.get("status") == "fail": - return {"status": "fail", "error": str(r.get("error", "test failed"))} + observed = [r for r in results if r.get("status") != "not_tested"] + if not observed: + return {"status": "not_tested"} - for r in results: + failures = [r for r in observed if r.get("status") == "fail"] + if failures: + errors = [str(r.get("error", "test failed")) for r in failures] + return {"status": "fail", "error": "; ".join(errors)} + + for r in observed: if r.get("status") == "not_applicable": return { "status": "not_applicable", "reason": str(r.get("reason", "not applicable")), } - if all(r.get("status") == "pass" for r in results): + if all(r.get("status") == "pass" for r in observed): return {"status": "pass"} return {"status": "not_tested"}