Surface lockfile discovery failures via typed exceptions (#79)

docs/developers-guide.md

@@ -365,8 +365,10 @@ this helper for consistent path handling, including `lading.cli`,
 
 `discover_tracked_lockfiles(workspace_root, runner)` filters git-tracked
 `Cargo.lock` files outside `target/` with adjacent `Cargo.toml` manifests.
-Private helpers `_handle_git_ls_files_failure` and `_lockfiles_with_manifests`
-perform the error-handling and path-filtering passes respectively.
+Private helpers `_raise_git_ls_files_failure` and `_lockfiles_with_manifests`
+perform the error-handling and path-filtering passes respectively. Discovery
+raises `NotAGitRepositoryError` when the workspace is not under git control;
+callers that should skip that condition own the skip policy at their boundary.
 
 Lockfile regeneration after `lading bump` is owned by
 `lading.commands.bump_lockfiles.regenerate_lockfiles`, which runs

lading/commands/lockfile.py

@@ -64,6 +64,15 @@ class LockfileDiscoveryError(LadingError):
     """Raised when git cannot list tracked lockfiles."""
 
 
+class NotAGitRepositoryError(LockfileDiscoveryError):
+    """Raised when lockfile discovery targets a directory outside git control.
+
+    Callers that treat a non-git workspace as "nothing to validate" should
+    catch this subclass and decide the skip policy themselves; discovery no
+    longer hides the condition behind a warning and an empty result.
+    """
+
+
 @dc.dataclass(frozen=True, slots=True)
 class LockfileFreshness:
     """Result from validating a lockfile under Cargo's locked mode."""
@@ -73,22 +82,17 @@ class LockfileFreshness:
     detail: str = ""
 
 
-def _handle_git_ls_files_failure(
+def _raise_git_ls_files_failure(
     exit_code: int,
     stdout: str,
     stderr: str,
     workspace_root: Path,
-) -> tuple[Path, ...] | None:
-    """Return ``None`` for git success, or an empty result for git failure."""
-    if exit_code == 0:
-        return None
+) -> typ.NoReturn:
+    """Raise the typed discovery error for a failed ``git ls-files`` call."""
     detail = command_detail(stdout, stderr)
     if "not a git repository" in detail.lower():
-        LOGGER.warning(
-            "Skipping Cargo.lock discovery because %s is not a git repository",
-            workspace_root,
-        )
-        return ()
+        message = f"{workspace_root} is not a git repository"
+        raise NotAGitRepositoryError(message)
     # Unlike the other sites, git may exit non-zero with no output at all, so
     # fall back to the status code before handing the detail to append_detail.
     fallback = f"git ls-files exited with status {exit_code}"
@@ -146,24 +150,30 @@ def discover_tracked_lockfiles(
     -------
     tuple[Path, ...]
         Git-tracked ``Cargo.lock`` files outside any ``target`` directory and
-        with an adjacent ``Cargo.toml`` manifest. The helper
-        :func:`_handle_git_ls_files_failure` handles git failures, and
+        with an adjacent ``Cargo.toml`` manifest.
         :func:`_lockfiles_with_manifests` applies manifest and path filtering.
 
+    Raises
+    ------
+    NotAGitRepositoryError
+        If ``workspace_root`` is not under git control. Callers own the skip
+        policy for that condition.
+    LockfileDiscoveryError
+        If ``git ls-files`` fails for any other reason.
+
     Notes
     -----
-    If ``workspace_root`` is not a git repository, discovery logs a warning
-    through :func:`_handle_git_ls_files_failure` and returns an empty tuple.
+    Filesystem access is confined to the injected ``manifest_exists`` port;
+    git access is confined to the injected ``runner``. The function performs
+    no direct I/O of its own, so integration tests can exercise it against
+    real directories and unit tests can substitute both ports.
     """
     exit_code, stdout, stderr = runner(
         ("git", "ls-files", "**/Cargo.lock", "Cargo.lock"),
         cwd=workspace_root,
     )
-    error_result = _handle_git_ls_files_failure(
-        exit_code, stdout, stderr, workspace_root
-    )
-    if error_result is not None:
-        return error_result
+    if exit_code != 0:
+        _raise_git_ls_files_failure(exit_code, stdout, stderr, workspace_root)
     lockfiles = _lockfiles_with_manifests(stdout, workspace_root, manifest_exists)
     if lockfiles:
         # Skip a zero-amount increment: it would create a 0-valued counter key

lading/commands/publish_preflight.py

@@ -33,7 +33,10 @@
 import typing as typ
 from pathlib import Path
 
-from lading.commands.lockfile import CargoLockfileInspectionRepository
+from lading.commands.lockfile import (
+    CargoLockfileInspectionRepository,
+    NotAGitRepositoryError,
+)
 from lading.commands.publish_diagnostics import _append_compiletest_diagnostics
 from lading.commands.publish_errors import PublishPreflightError
 from lading.commands.publish_execution import _invoke
@@ -266,7 +269,16 @@ def _validate_lockfile_freshness(
     holds a command runner or knows how lockfiles are located and validated
     (issue #82).
     """
-    tracked = repository.discover_tracked_lockfiles(workspace_root)
+    try:
+        tracked = repository.discover_tracked_lockfiles(workspace_root)
+    except NotAGitRepositoryError:
+        # Skip policy lives here, not in discovery: a non-git workspace has
+        # no tracked lockfiles to validate, so pre-flight continues.
+        LOGGER.warning(
+            "Skipping lockfile freshness validation: %s is not a git repository",
+            workspace_root,
+        )
+        return
     stale_lockfiles = _collect_stale_lockfiles(tracked, repository)
 
     if not stale_lockfiles:

tests/integration/test_lockfile_discovery.py

@@ -0,0 +1,102 @@
+"""Integration tests for lockfile discovery against real git repositories.
+
+Issue #79: these tests exercise :func:`discover_tracked_lockfiles` through
+the real subprocess runner and real temporary directories, without stubbing
+the internals, so the git and filesystem integrations are covered rather
+than mocked layouts.
+"""
+
+from __future__ import annotations
+
+import shutil
+import subprocess
+import typing as typ
+
+import pytest
+
+from lading.commands import lockfile
+from lading.runtime import subprocess_runner
+
+if typ.TYPE_CHECKING:
+    from pathlib import Path
+
+pytestmark = pytest.mark.timeout(60)
+
+
+_GIT_EXECUTABLE = shutil.which("git") or "/usr/bin/git"
+
+
+def _git(repo: Path, *args: str) -> None:
+    """Run a git command in ``repo`` and fail loudly on error."""
+    subprocess.run(  # noqa: S603 - fixed argv, test fixture setup
+        (_GIT_EXECUTABLE, *args),
+        cwd=repo,
+        check=True,
+        capture_output=True,
+        env={
+            "GIT_AUTHOR_NAME": "test",
+            "GIT_AUTHOR_EMAIL": "test@example.com",
+            "GIT_COMMITTER_NAME": "test",
+            "GIT_COMMITTER_EMAIL": "test@example.com",
+            "PATH": "/usr/bin:/bin",
+            "HOME": str(repo),
+        },
+    )
+
+
+def _init_repo(repo: Path) -> None:
+    """Initialise a git repository with deterministic identity settings."""
+    repo.mkdir(parents=True, exist_ok=True)
+    _git(repo, "init", "--quiet")
+
+
+def _add_crate(repo: Path, relative: str, *, with_manifest: bool = True) -> Path:
+    """Create a crate directory with a Cargo.lock (and optional manifest)."""
+    crate_dir = repo / relative if relative else repo
+    crate_dir.mkdir(parents=True, exist_ok=True)
+    lock = crate_dir / "Cargo.lock"
+    lock.write_text("# lock\n", encoding="utf-8")
+    if with_manifest:
+        (crate_dir / "Cargo.toml").write_text(
+            '[package]\nname = "x"\nversion = "0.1.0"\n', encoding="utf-8"
+        )
+    return lock
+
+
+def test_discovery_returns_tracked_lockfiles_with_manifests(tmp_path: Path) -> None:
+    """Tracked lockfiles with adjacent manifests are discovered for real."""
+    repo = tmp_path / "repo"
+    _init_repo(repo)
+    root_lock = _add_crate(repo, "")
+    nested_lock = _add_crate(repo, "crates/alpha")
+    _add_crate(repo, "no-manifest", with_manifest=False)
+    target_lock = _add_crate(repo, "target/debug")
+    _git(repo, "add", "-A")
+
+    result = lockfile.discover_tracked_lockfiles(repo, subprocess_runner)
+
+    assert set(result) == {root_lock, nested_lock}
+    assert target_lock not in result
+
+
+def test_discovery_ignores_untracked_lockfiles(tmp_path: Path) -> None:
+    """Lockfiles not in the git index are not discovered."""
+    repo = tmp_path / "repo"
+    _init_repo(repo)
+    tracked = _add_crate(repo, "tracked")
+    _git(repo, "add", "tracked")
+    untracked = _add_crate(repo, "untracked")
+
+    result = lockfile.discover_tracked_lockfiles(repo, subprocess_runner)
+
+    assert tracked in result
+    assert untracked not in result
+
+
+def test_discovery_raises_for_non_git_directory(tmp_path: Path) -> None:
+    """A plain directory raises the typed not-a-repository error."""
+    workspace = tmp_path / "plain"
+    _add_crate(workspace, "")
+
+    with pytest.raises(lockfile.NotAGitRepositoryError):
+        lockfile.discover_tracked_lockfiles(workspace, subprocess_runner)

tests/unit/publish/test_preflight_lockfile_validation.py

@@ -10,6 +10,7 @@
 from __future__ import annotations
 
 import dataclasses as dc
+import logging
 import typing as typ
 from pathlib import Path
 
@@ -176,6 +177,50 @@ def test_validate_lockfile_freshness_surfaces_cargo_failures(tmp_path: Path) ->
         publish_preflight._validate_lockfile_freshness(tmp_path, repository=repository)
 
 
+def test_validate_lockfile_freshness_skips_non_git_workspaces(
+    tmp_path: Path,
+    caplog: pytest.LogCaptureFixture,
+) -> None:
+    """The skip policy for non-git workspaces lives at the pre-flight caller.
+
+    Issue #79: discovery raises ``NotAGitRepositoryError`` instead of hiding
+    the condition; pre-flight catches it at the port boundary, warns, and
+    continues without probing any lockfile.
+    """
+    caplog.set_level(logging.WARNING, logger="lading.commands.publish_preflight")
+
+    @dc.dataclass
+    class _NonGitRepository:
+        """Recording repository double that reports a non-git workspace."""
+
+        discovered_roots: list[Path] = dc.field(default_factory=list)
+
+        def discover_tracked_lockfiles(self, workspace_root: Path) -> tuple[Path, ...]:
+            """Record the call and raise the typed non-git discovery error."""
+            self.discovered_roots.append(workspace_root)
+            message = f"{workspace_root} is not a git repository"
+            raise lockfile.NotAGitRepositoryError(message)
+
+        def validate_lockfile_freshness(
+            self, manifest_path: Path
+        ) -> lockfile.LockfileFreshness:
+            """Fail: no lockfile should be validated for a non-git workspace."""
+            pytest.fail(
+                f"no lockfile should be validated for a non-git workspace: "
+                f"{manifest_path}"
+            )
+
+    repository = _NonGitRepository()
+
+    publish_preflight._validate_lockfile_freshness(tmp_path, repository=repository)
+
+    assert repository.discovered_roots == [tmp_path]
+    assert any(
+        "Skipping lockfile freshness validation" in message
+        for message in caplog.messages
+    )
+
+
 def test_validate_lockfile_freshness_classifies_every_lockfile(
     tmp_path: Path,
 ) -> None:

tests/unit/test_lockfile.py

@@ -183,10 +183,10 @@ def manifest_exists(manifest_path: Path) -> bool:
     assert probed == [tmp_path / "Cargo.toml", tmp_path / "nested" / "Cargo.toml"]
 
 
-def test_discover_tracked_lockfiles_handles_non_git_directory(
+def test_discover_tracked_lockfiles_raises_for_non_git_directory(
     tmp_path: Path,
 ) -> None:
-    """Non-git workspaces do not abort lockfile discovery."""
+    """Non-git workspaces surface a typed error instead of a silent skip."""
     (tmp_path / "Cargo.lock").write_text("", encoding="utf-8")
 
     def runner(
@@ -197,11 +197,10 @@ def runner(
     ) -> tuple[int, str, str]:
         return 128, "", "fatal: not a git repository"
 
-    result = lockfile.discover_tracked_lockfiles(tmp_path, runner)
-    assert result == (), (
-        "discovery should not abort on non-git errors; "
-        f"expected empty tuple, got {result!r}"
-    )
+    with pytest.raises(
+        lockfile.NotAGitRepositoryError, match="is not a git repository"
+    ):
+        lockfile.discover_tracked_lockfiles(tmp_path, runner)
 
 
 def test_discover_tracked_lockfiles_raises_on_git_failure(tmp_path: Path) -> None: