📝 CodeRabbit Chat: Implement requested code changes

lading/commands/bump_lockfiles.py

@@ -9,6 +9,7 @@
 
 import collections.abc as cabc
 import logging
+import shlex
 import time
 from pathlib import Path
 
@@ -60,37 +61,94 @@ def regenerate_lockfiles(
     ------
     LockfileRegenerationError
         If any configured manifest path is invalid (outside the workspace or
-        not named ``Cargo.toml``), or if ``cargo update --workspace`` exits
-        non-zero for any manifest.
+        not named ``Cargo.toml``), or — after every manifest has been
+        attempted — if ``cargo update --workspace`` failed for one or more
+        manifests. The aggregated message lists each failed manifest with a
+        repair command.
 
     Notes
     -----
-    **Partial-update semantics:** regeneration is not atomic. If ``cargo``
-    fails for the *n*-th manifest, the first *n - 1* lockfiles have already
-    been updated on disk and will not be rolled back. Callers that require
-    atomic guarantees must implement their own rollback (see issue ``#84``).
+    **Partial-update semantics:** regeneration is not atomic and successful
+    updates are not rolled back when a later manifest fails. Every manifest
+    is attempted (issue #84), so a single cargo failure does not leave
+    unrelated lockfiles silently stale; the aggregated error tells the
+    operator exactly which lockfiles still need repair and how.
     """
     command_runner = subprocess_runner if runner is None else runner
     manifests = _resolve_manifest_paths(workspace_root, lockfile_manifests)
     started_at = time.perf_counter()
     _LOGGER.info("Regenerating %d Cargo lockfile(s)", len(manifests))
+    lockfiles, failures = _collect_lockfile_results(
+        workspace_root, manifests, command_runner
+    )
+    _raise_if_failures(failures)
+    _LOGGER.info(
+        "Regenerated %d Cargo lockfile(s) in %.3fs",
+        len(lockfiles),
+        time.perf_counter() - started_at,
+    )
+    return tuple(lockfiles)
+
+
+def _collect_lockfile_results(
+    workspace_root: Path,
+    manifests: cabc.Sequence[Path],
+    runner: CommandRunner,
+) -> tuple[list[Path], list[tuple[Path, LockfileRegenerationError]]]:
+    """Attempt every manifest and return successful lockfiles and failures."""
     lockfiles: list[Path] = []
+    failures: list[tuple[Path, LockfileRegenerationError]] = []
     for manifest in manifests:
         manifest_started_at = time.perf_counter()
         _LOGGER.info("Regenerating Cargo lockfile for %s", manifest)
-        _run_workspace_lockfile_update(workspace_root, manifest, command_runner)
+        try:
+            _run_workspace_lockfile_update(workspace_root, manifest, runner)
+        except LockfileRegenerationError as exc:
+            # Keep going: attempting the remaining manifests gives the
+            # operator one aggregated repair list instead of a re-run per
+            # failure (issue #84).
+            _LOGGER.exception("Cargo lockfile regeneration failed")
+            failures.append((manifest, exc))
+            continue
         _LOGGER.info(
             "Regenerated Cargo lockfile for %s in %.3fs",
             manifest,
             time.perf_counter() - manifest_started_at,
         )
         lockfiles.append(manifest.parent / "Cargo.lock")
-    _LOGGER.info(
-        "Regenerated %d Cargo lockfile(s) in %.3fs",
-        len(lockfiles),
-        time.perf_counter() - started_at,
+    return lockfiles, failures
+
+
+def _raise_if_failures(
+    failures: list[tuple[Path, LockfileRegenerationError]],
+) -> None:
+    """Raise an aggregated error when one or more manifest updates failed."""
+    if not failures:
+        return
+    # Chain from the first underlying failure so diagnostics (for
+    # example a missing cargo executable) survive the aggregation.
+    primary = failures[0][1]
+    cause = primary.__cause__ if primary.__cause__ is not None else primary
+    raise LockfileRegenerationError(
+        _build_aggregate_failure_message(failures)
+    ) from cause
+
+
+def _build_aggregate_failure_message(
+
+) -> str:
+    """Return the aggregated operator-facing regeneration failure message."""
+    header = (
+        f"Cargo lockfile regeneration failed for {len(failures)} manifest(s). "
+        "Manifests already carry the new version, so the workspace is "
+        "inconsistent until each lockfile below is repaired:"
     )
-    return tuple(lockfiles)
+    lines = [header]
+    for manifest, error in failures:
+        lines.append(f"- {error}")
+        quoted_manifest = shlex.quote(str(manifest))
+        lines.append(f"  cargo update --workspace --manifest-path {quoted_manifest}")
+    return "\n".join(lines)
 
 
 def _resolve_manifest_paths(

tests/unit/__snapshots__/test_bump_lockfiles.ambr

@@ -0,0 +1,10 @@
+# serializer version: 1
+# name: test_regenerate_lockfiles_aggregates_multiple_failures
+  '''
+  Cargo lockfile regeneration failed for 2 manifest(s). Manifests already carry the new version, so the workspace is inconsistent until each lockfile below is repaired:
+  - Cargo lockfile regeneration failed for <workspace>/a/Cargo.toml with exit code 101: error: dependency conflict
+    cargo update --workspace --manifest-path <workspace>/a/Cargo.toml
+  - Cargo lockfile regeneration failed for <workspace>/b/Cargo.toml with exit code 101: error: dependency conflict
+    cargo update --workspace --manifest-path <workspace>/b/Cargo.toml
+  '''
+# ---

tests/unit/test_bump_lockfiles.py

@@ -6,13 +6,14 @@
 import dataclasses as dc
 import pathlib
 import typing as typ
+from pathlib import Path
 
 import pytest
 
 from lading.commands import bump_lockfiles
 
 if typ.TYPE_CHECKING:
-    from pathlib import Path
+    from syrupy.assertion import SnapshotAssertion
 
 
 @dc.dataclass(frozen=True, slots=True)
@@ -267,3 +268,87 @@ def failing_runner(
         )
 
     assert isinstance(exc_info.value.__cause__, OSError)
+
+
+# ---------------------------------------------------------------------------
+# Aggregated failure handling (issue #84)
+# ---------------------------------------------------------------------------
+
+
+def _selective_failure_runner(
+    failing_manifests: set[Path],
+) -> tuple[cabc.Callable[..., tuple[int, str, str]], list[Path]]:
+    """Return a runner failing for ``failing_manifests`` and its call log."""
+    attempted: list[Path] = []
+
+    def runner(
+        command: cabc.Sequence[str],
+        *,
+        cwd: Path | None = None,
+    ) -> tuple[int, str, str]:
+        del cwd
+        manifest = Path(command[-1])
+        attempted.append(manifest)
+        if manifest in failing_manifests:
+            return 101, "", "error: dependency conflict"
+        return 0, "", ""
+
+    return runner, attempted
+
+
+def test_regenerate_lockfiles_attempts_all_manifests_after_failure(
+    tmp_path: Path,
+) -> None:
+    """A mid-list cargo failure does not skip the remaining manifests."""
+    for name in ("a", "b"):
+        (tmp_path / name).mkdir()
+        (tmp_path / name / "Cargo.toml").write_text("", encoding="utf-8")
+    (tmp_path / "Cargo.toml").write_text("", encoding="utf-8")
+    failing = (tmp_path / "a" / "Cargo.toml").resolve()
+    runner, attempted = _selective_failure_runner({failing})
+
+    with pytest.raises(bump_lockfiles.LockfileRegenerationError) as excinfo:
+        bump_lockfiles.regenerate_lockfiles(
+            tmp_path,
+            ("a/Cargo.toml", "b/Cargo.toml"),
+            runner=runner,
+        )
+
+    assert attempted == [
+        (tmp_path / "Cargo.toml").resolve(),
+        failing,
+        (tmp_path / "b" / "Cargo.toml").resolve(),
+    ]
+    message = str(excinfo.value)
+    assert "failed for 1 manifest(s)" in message
+    assert str(failing) in message
+    assert f"cargo update --workspace --manifest-path {failing}" in message
+    assert str((tmp_path / "b" / "Cargo.toml").resolve()) not in message
+
+
+def test_regenerate_lockfiles_aggregates_multiple_failures(
+    tmp_path: Path, snapshot: SnapshotAssertion
+) -> None:
+    """Every failed manifest is listed once with its repair command."""
+    for name in ("a", "b"):
+        (tmp_path / name).mkdir()
+        (tmp_path / name / "Cargo.toml").write_text("", encoding="utf-8")
+    (tmp_path / "Cargo.toml").write_text("", encoding="utf-8")
+    failing = {
+        (tmp_path / "a" / "Cargo.toml").resolve(),
+        (tmp_path / "b" / "Cargo.toml").resolve(),
+    }
+    runner, _ = _selective_failure_runner(failing)
+
+    with pytest.raises(bump_lockfiles.LockfileRegenerationError) as excinfo:
+        bump_lockfiles.regenerate_lockfiles(
+            tmp_path,
+            ("a/Cargo.toml", "b/Cargo.toml"),
+            runner=runner,
+        )
+
+    message = str(excinfo.value)
+    assert "failed for 2 manifest(s)" in message
+    for manifest in failing:
+        assert message.count(f"--manifest-path {manifest}") == 1
+    assert snapshot == message.replace(str(tmp_path), "<workspace>")