fix(dependency-groups): collect errors from sibling includes after a first parse error

[Str0k]

When a resolution collects an error while parsing one included group, any later sibling include of that resolution is silently skipped: its group is never parsed and its own errors never surface. For example, with root including bad (containing an invalid requirement) and wrapper (which includes also-bad containing another invalid requirement), resolve_dependency_groups raised with only bad's InvalidRequirement; also-bad's error was lost. After the fix, the same resolution reports both errors, matching the documented aggregation behavior covered by existing tests for direct sibling errors.

DependencyGroupResolver._parse_group ended with if errors.errors: return (), which tested the error collector shared across the whole resolution rather than errors produced by this group's own parse. Once any earlier error existed, a clean group's parse result was discarded and never cached, so sibling includes resolved afterwards were never followed. The guard now compares the collector's length before and after parsing this group, so only groups that themselves produced errors are treated as failed and left uncached; all prior behaviors (repeat errors on cyclic groups, duplicate-normalized-name detection, cache semantics) are preserved.

Validation:

• Regression on unchanged base: 1 assertion failure(s); with patch: 2 tests, exit 0.
• Full suite: base 62437 tests (exit 1); patch 62437 tests (exit 0).
• Repository checks: coverage_gaps: exit 0, license_header: exit 1, mypy: exit 0, ruff: exit 0, ruff_format: exit 0, typos: exit 0.

AI assistance: implementation and independent review used Hermes with GLM 5.3. Test evidence was reproduced in clean checkouts. This does not represent a human review.

[henryiii]

LGTM, pinging @sirosen for a review.

[sirosen]

Thanks for tagging me in for review! It took a little bit for me to get a clear evening to play around with this branch.

IMO a real defect has been identified (not collecting all errors), but I'm not convinced about the fix being proposed.

One of the effects of addressing the issue in this way is that a diamond-shaped tree with an error at the bottom will produce two sub-errors which are duplicates.

Consider this snippet:

from packaging.dependency_groups import DependencyGroupResolver
data = {"group1": [{"include-group": "err"}], "group2": [{"include-group": "err"}], "err": ["!badval"], "root": [{"include-group": "group1"}, {"include-group": "group2"}]}
r = DependencyGroupResolver(data)
r.resolve('root')

On main you get one error while on this branch you get two.

I think one error is correct for this data shape? So I'm looking at this a little to see if there's a better fix possible.

[sirosen]

I spent some time fixing this further and thinking. I noticed three things about this branch:

1. The tests should not be in a new module; they should go into test_dependency_groups.py.
2. I'm not sure the use of a shared fixture and the type hints for data shapes (e.g., dict[str, list[str | dict[str, str]]]) are helpful in the tests -- I think this is currently harmful to readability.
3. The issue I flagged above is newly surfaced, but happens at the layer above where the change is applied. There's probably some version of it which you can trigger in main today.

I worked up a changeset that fixes this by adding another tracking datastructure.

sample patch

diff --git a/src/packaging/dependency_groups.py b/src/packaging/dependency_groups.py
index eafb80a..ec8f2c4 100644
--- a/src/packaging/dependency_groups.py
+++ b/src/packaging/dependency_groups.py
@@ -155,10 +155,14 @@ class DependencyGroupResolver:
         with _ErrorCollector().on_exit(
             f"[dependency-groups] data for {group!r} was malformed"
         ) as errors:
-            return self._resolve(group, group, errors)
+            return self._resolve(group, group, errors, set())

     def _resolve(
-        self, group: str, requested_group: str, errors: _ErrorCollector
+        self,
+        group: str,
+        requested_group: str,
+        errors: _ErrorCollector,
+        seen_error_groups: set[str],
     ) -> tuple[Requirement, ...]:
         """
         This is a helper for cached resolution to strings. It preserves the name of the
@@ -168,7 +172,14 @@ class DependencyGroupResolver:
         :param group: The normalized name of the group to resolve.
         :param requested_group: The group which was used in the original, user-facing
             request.
+        :param errors: An error collector in active use.
+        :param seen_error_groups: An ephemeral set of group names which have already
+            resolved to errors in the context of the current call. Used to avoid
+            repeating errors for a single group within a call.
         """
+        if group in seen_error_groups:
+            return ()
+
         if group in self._resolve_cache:
             return self._resolve_cache[group]

@@ -199,7 +210,9 @@ class DependencyGroupResolver:
                         group,
                     )
                     resolved_group.extend(
-                        self._resolve(include_group, requested_group, errors)
+                        self._resolve(
+                            include_group, requested_group, errors, seen_error_groups
+                        )
                     )
             else:  # pragma: no cover
                 raise NotImplementedError(
@@ -210,6 +223,7 @@ class DependencyGroupResolver:
         # cache the result
         # this ensures that repeated access to a cyclic group will raise multiple errors
         if errors.errors:
+            seen_error_groups.add(group)
             return ()

         self._resolve_cache[group] = tuple(resolved_group)
diff --git a/tests/test_dependency_groups.py b/tests/test_dependency_groups.py
index 4f291d1..9eee642 100644
--- a/tests/test_dependency_groups.py
+++ b/tests/test_dependency_groups.py
@@ -130,7 +130,7 @@ def test_expand_contract_model_only_does_inner_lookup_once() -> None:
         # each of the `mid` nodes will call resolution with `contract`, but only the
         # first of those evaluations should call for resolution of `leaf` -- after that,
         # `contract` will be in the cache and `leaf` will not need to be resolved
-        spy.assert_any_call("leaf", "root", unittest.mock.ANY)
+        spy.assert_any_call("leaf", "root", unittest.mock.ANY, unittest.mock.ANY)
         leaf_calls = [c for c in spy.mock_calls if c.args[0] == "leaf"]
         assert len(leaf_calls) == 1

@@ -541,3 +541,15 @@ def test_resolution_can_capture_multiple_errors_at_once() -> None:
         TypeError,
         match=r"Dependency group 'invalid-type' contained a string rather than a list.",
     )
+
+
+def test_diamond_shaped_dependencies_result_in_one_error() -> None:
+    data = {
+        "root": [{"include-group": "group1"}, {"include-group": "group2"}],
+        "group1": [{"include-group": "err"}],
+        "group2": [{"include-group": "err"}],
+        "err": ["invalid requirement"],
+    }
+    with pytest.raises(ExceptionGroup) as excinfo:
+        resolve_dependency_groups(data, "root")
+    assert len(excinfo.value.exceptions) == 1

In terms of external behavior, good, but I don't like where this is all going: more tracking structures without a good overarching strategy. The original code was written with a crash-only design that failed on the first issue, and we retrofitted it with the exception groups. I still haven't figured out what I want to do with this, but the current code has the feeling of being caught between worlds, and I'd like to resolve it.

[henryiii]

Thanks @sirosen! I pushed your fix and have a possible followup to simplify.