From 07d73aecefc390be2a8f2cd5899eeb2a363c9d90 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Tue, 1 Sep 2026 21:27:46 +0200 Subject: [PATCH] fix(meta/complexity): decide coverage by what the coverer actually pulls The greedy set-cover retires a role's variant-0 row when an earlier row of another role embeds it, on the premise that embedding pulls the provider at variant 0. The premise is a property of the pair, not of the coverer: a job's round index is the primary role's own variant index, and plan_dev_inventory_matrix hands each pulled-in dependency 'round_index if round_index < its variant count else 0'. A variant 3 coverer therefore pulls a 3-variant provider at 0 and a 14-variant provider at 3. Coverage now asks that question per pair instead of assuming the answer. Nothing changes on the current tree, where 53 rows are covered before and after: the discovery sort ends on 'asc variant', so a role's variant-0 row is always green before its higher ones and gets to cover first. That is a property of a sort key in default.env, not of this code, and 2633 embed pairs would fall the wrong way without it. Co-Authored-By: Claude Opus 5 (1M context) --- cli/meta/roles/applications/complexity/cli.py | 33 ++++++++++++++--- .../complexity/test_complexity.py | 35 +++++++++++++++---- 2 files changed, 57 insertions(+), 11 deletions(-) diff --git a/cli/meta/roles/applications/complexity/cli.py b/cli/meta/roles/applications/complexity/cli.py index 9e7249e371..7a9d3e2412 100644 --- a/cli/meta/roles/applications/complexity/cli.py +++ b/cli/meta/roles/applications/complexity/cli.py @@ -8,6 +8,7 @@ import sys from typing import TYPE_CHECKING, Any from utils import PROJECT_ROOT +from utils.cache.applications import get_variants from utils.roles.applications.topics import overridden_providers if TYPE_CHECKING: @@ -279,6 +280,7 @@ def _apply_sort(rows: list[ComplexityRow], sort_spec: list[tuple[str, bool]]) -> def _mark_covered( rows: list[ComplexityRow], overridden: Mapping[tuple[str, int], set[str]] | None = None, + variant_counts: Mapping[str, int] | None = None, ) -> list[ComplexityRow]: """Assign each sorted row its numeric ``id`` (its position in sort order) and its ``covered_by`` via a greedy set-cover: the first row is green, and @@ -291,10 +293,15 @@ def _mark_covered( coverer's deploy genuinely brings this row up. Two variants of the same role never cover each other (a role never embeds itself). - Coverage is variant-aware: when a service.yml declares another service - enabled+shared it always pulls that provider's variant 0, so only a row's - variant-0 (or whole-role) form can be covered. A variant > 0 row is never - covered and is always green; it may still cover other roles' rows. + Coverage is variant-aware: only a row's variant-0 (or whole-role) form can + be covered, because a coverer only ever brings a provider up at variant 0. + Which coverers do that is not a property of the coverer alone. A job's + round index is the primary role's own variant index, and + ``inventory.planner.plan_dev_inventory_matrix`` hands each pulled-in + dependency ``round_index if round_index < its variant count else 0``. So a + variant N row covers a provider exactly when N lands past that provider's + last variant and falls back to 0 - which is why ``variant_counts`` is + needed here: N alone does not say it. A variant that dictates a provider's own config is not evidence about that provider in any variant: the deploy ran a configuration the provider does @@ -305,8 +312,16 @@ def _mark_covered( rows: the complexity rows, in ranking order. overridden: ``{(role, variant): {provider role, ...}}``, or None when no variant dictates anything. + variant_counts: ``{role: how many variants it declares}``. A role + missing from it counts as one, which is the whole-role case. """ demands = overridden or {} + counts = variant_counts or {} + + def pulls_at_zero(coverer_variant: int | None, provider: str) -> bool: + index = coverer_variant or 0 + return index == 0 or index >= counts.get(provider, 1) + green: list[tuple[int, str, int | None, set[str]]] = [] out: list[ComplexityRow] = [] for index, row in enumerate(rows, start=1): @@ -319,6 +334,7 @@ def _mark_covered( if gname != row.name and row.name in gset and row.name not in demands.get((gname, gvariant or 0), ()) + and pulls_at_zero(gvariant, row.name) ), None, ) @@ -374,7 +390,14 @@ def main(argv: list[str] | None = None) -> int: rows = attach_siblings(rows) _apply_sort(rows, sort_spec) - rows = _mark_covered(rows, overridden_providers(roles_dir)) + rows = _mark_covered( + rows, + overridden_providers(roles_dir), + { + role: max(1, len(entries or [{}])) + for role, entries in get_variants(roles_dir=roles_dir).items() + }, + ) _apply_sort(rows, sort_spec) rows = [r._replace(row=line) for line, r in enumerate(rows, start=1)] diff --git a/tests/unit/python/cli/meta/roles/applications/complexity/test_complexity.py b/tests/unit/python/cli/meta/roles/applications/complexity/test_complexity.py index ec2d862733..26e35c3170 100644 --- a/tests/unit/python/cli/meta/roles/applications/complexity/test_complexity.py +++ b/tests/unit/python/cli/meta/roles/applications/complexity/test_complexity.py @@ -944,24 +944,47 @@ class TestCoveredBy(unittest.TestCase): ) self.assertEqual([m.covered_by for m in marked], [0, 0, 2, 0]) - def test_variant_gt0_can_still_cover_other_roles(self) -> None: + def test_variant_gt0_covers_a_provider_it_pulls_at_zero(self) -> None: marked = _mark_covered( [ _row("Z", []), _row("A", ["B"], variant=2), _row("B", [], variant=0), - ] + ], + variant_counts={"B": 1}, + ) + self.assertEqual([m.covered_by for m in marked], [0, 0, 2]) + + def test_variant_gt0_does_not_cover_a_provider_with_that_variant(self) -> None: + marked = _mark_covered( + [ + _row("Z", []), + _row("A", ["B"], variant=2), + _row("B", [], variant=0), + ], + variant_counts={"B": 5}, + ) + self.assertEqual([m.covered_by for m in marked], [0, 0, 0]) + + def test_variant_zero_covers_whatever_its_provider_count(self) -> None: + marked = _mark_covered( + [ + _row("Z", []), + _row("A", ["B"], variant=0), + _row("B", [], variant=0), + ], + variant_counts={"B": 9}, ) self.assertEqual([m.covered_by for m in marked], [0, 0, 2]) def test_a_variant_that_overrides_a_provider_does_not_cover_it(self) -> None: - rows = [_row("A", ["B", "C"], variant=1), _row("B", []), _row("C", [])] - marked = _mark_covered(rows, {("A", 1): {"B"}}) + rows = [_row("A", ["B", "C"], variant=0), _row("B", []), _row("C", [])] + marked = _mark_covered(rows, {("A", 0): {"B"}}) self.assertEqual([m.covered_by for m in marked], [0, 0, 1]) def test_the_override_only_bars_the_provider_it_names(self) -> None: - rows = [_row("A", ["B"], variant=1), _row("B", [])] - marked = _mark_covered(rows, {("A", 1): {"C"}}) + rows = [_row("A", ["B"], variant=0), _row("B", [])] + marked = _mark_covered(rows, {("A", 0): {"C"}}) self.assertEqual([m.covered_by for m in marked], [0, 1]) def test_an_override_in_another_variant_leaves_coverage_alone(self) -> None: