Enhance LFS error handling and logging in fetch and push operations (#8)

* Add error handling for LFS fetch and push operations with logging

* Enhance LFS handling and review logging in absorb process

* Fix LFS fetch references in absorb and publish methods

Author: ardiloot

SPEC.md

@@ -150,7 +150,7 @@ pubgate supports repositories that use Git LFS. LFS support is auto-detected via
 
 **How it works:** LFS-tracked files are stored as pointer files in git. pubgate reads and writes these pointers as-is; they pass through the snapshot, stage, and publish pipelines without modification. When files are staged with `git add`, git's clean/smudge filters handle the LFS encoding automatically via `.gitattributes`.
 
-**LFS object transfer:** pubgate runs `git lfs fetch` during command startups (absorb, publish) to ensure LFS objects are locally cached, and `git lfs push` after pushing branches to transfer LFS objects to the destination remote's LFS server.
+**LFS object transfer:** pubgate runs `git lfs fetch` before file operations that need blob content (absorb, publish), skipping the fetch when there is no work to do (e.g. already up to date) or during `--dry-run`. After pushing branches, `git lfs push` transfers LFS objects to the destination remote's LFS server.
 
 **Limitations:**
 - **LFS files are treated as binary**: they are never merged during `absorb` (copied/overwritten instead) and never scrubbed for `BEGIN-INTERNAL`/`END-INTERNAL` markers during `stage`. Do not place internal markers inside LFS-tracked files; use ignore patterns in `pubgate.toml` to exclude sensitive LFS files from publication.

pubgate/absorb.py

@@ -67,6 +67,7 @@ def absorb_commit_message(
     last_absorbed: str,
     public_head: str,
     conflicted: list[str] | None = None,
+    needs_review: list[str] | None = None,
 ) -> str:
     subject = f"pubgate: absorb public changes {last_absorbed[:7]}..{public_head[:7]}"
     commits = git.log_oneline(last_absorbed, public_head)
@@ -80,6 +81,11 @@ def absorb_commit_message(
         lines.append("CONFLICTS (resolve before merging):")
         for path in conflicted:
             lines.append(f"  {path}")
+    if needs_review:
+        lines.append("")
+        lines.append("Review manually:")
+        for item in needs_review:
+            lines.append(f"  {item}")
     return "\n".join(lines)
 
 
@@ -95,6 +101,40 @@ def _read_text_at_ref(git: GitRepo, ref: str, path: str) -> str | None:
     return data.decode("utf-8")
 
 
+def _log_review_diff(git: GitRepo, public_ref: str, path: str, local_path: Path) -> None:
+    """Log debug info comparing local vs public version of a file needing review."""
+    try:
+        public_bytes = git.read_file_at_ref_bytes(public_ref, path)
+        local_bytes = local_path.read_bytes() if local_path.exists() else None
+        pub_size = len(public_bytes) if public_bytes is not None else 0
+        loc_size = len(local_bytes) if local_bytes is not None else 0
+        same = public_bytes == local_bytes
+        logger.debug("    public: %d bytes, local: %d bytes, identical: %s", pub_size, loc_size, same)
+        if same:
+            return
+        # For text files, show a unified diff
+        if public_bytes is not None and local_bytes is not None:
+            try:
+                pub_text = public_bytes.decode("utf-8")
+                loc_text = local_bytes.decode("utf-8")
+            except UnicodeDecodeError:
+                return
+            import difflib
+
+            diff = difflib.unified_diff(
+                pub_text.splitlines(keepends=True),
+                loc_text.splitlines(keepends=True),
+                fromfile=f"public ({public_ref})",
+                tofile="local (working tree)",
+            )
+            diff_text = "".join(diff)
+            if diff_text:
+                for line in diff_text.splitlines():
+                    logger.debug("    %s", line)
+    except Exception:
+        pass  # best-effort debug logging
+
+
 def _apply_absorb_changes(
     git: GitRepo,
     base_sha: str,
@@ -116,11 +156,20 @@ def _apply_absorb_changes(
                 kind = git.classify_at_ref(public_ref, change.path)
                 if kind != "text":
                     label = "LFS file" if kind == "lfs" else "binary"
-                    actions.append(f"  {label} added on public (kept local version, review manually): {change.path}")
+                    public_bytes = git.read_file_at_ref_bytes(public_ref, change.path)
+                    local_bytes = local_path.read_bytes()
+                    if public_bytes == local_bytes:
+                        actions.append(f"  {label} (identical): {change.path}")
+                    else:
+                        actions.append(
+                            f"  {label} added on public (kept local version, review manually): {change.path}"
+                        )
+                        _log_review_diff(git, public_ref, change.path, local_path)
                 else:
                     theirs_content = _read_text_at_ref(git, public_ref, change.path)
                     if theirs_content is None:
                         actions.append(f"  added on public (kept local version, review manually): {change.path}")
+                        _log_review_diff(git, public_ref, change.path, local_path)
                         continue
                     # Try to find the published base: the scrubbed version of
                     # the file at the internal commit that was staged.
@@ -145,6 +194,7 @@ def _apply_absorb_changes(
                     else:
                         # No staged version; file wasn't published through pubgate
                         actions.append(f"  added on public (kept local, review manually): {change.path}")
+                        _log_review_diff(git, public_ref, change.path, local_path)
             else:
                 is_binary = git.copy_file_from_ref(public_ref, change.path)
                 if is_binary:
@@ -161,6 +211,7 @@ def _apply_absorb_changes(
         elif change.is_delete:
             if (git.repo_dir / change.path).exists():
                 actions.append(f"  deleted on public (kept locally, review manually): {change.path}")
+                logger.debug("    local file exists at %s", git.repo_dir / change.path)
 
         elif change.is_rename:
             old_path = change.old_path or ""
@@ -192,6 +243,7 @@ def _merge_file(
         git.write_file_and_stage_bytes(path, theirs_bytes)
         label = "LFS file" if is_lfs_pointer(theirs_bytes) else "binary"
         actions.append(f"  {label} changed on public (replaced locally, review manually): {path}")
+        _log_review_diff(git, public_ref, path, git.repo_dir / path)
         return
 
     # Use the scrubbed staged content as merge base when available.

pubgate/core.py

@@ -152,6 +152,8 @@ def _bootstrap_work() -> bool:
             )
             return
 
+        git.lfs_fetch(cfg.public_remote, public_head)
+
         def _absorb_work() -> bool:
             actions = resolve_and_apply(cfg, git, last_absorbed, public_head)
             if actions:
@@ -163,7 +165,8 @@ def _absorb_work() -> bool:
                     logger.info("%s", a)
             git.write_file_and_stage(cfg.absorb_state_file, public_head + "\n")
             conflicted = [a.split(": ", 1)[1] for a in actions if "CONFLICTS" in a]
-            msg = absorb_commit_message(git, last_absorbed, public_head, conflicted)
+            needs_review = [a.strip() for a in actions if "review manually" in a]
+            msg = absorb_commit_message(git, last_absorbed, public_head, conflicted, needs_review)
             sha = git.commit(msg)
             logger.info("Committed on %s (%s %s)", cfg.internal_absorb_branch, sha[:7], msg.split("\n", 1)[0])
             return True
@@ -198,7 +201,9 @@ def stage(self, *, dry_run: bool = False, force: bool = False, no_pr: bool = Fal
         origin_preview_ref = f"origin/{cfg.internal_approved_branch}"
 
         ignore_patterns = list(cfg.ignore)
-        snapshot = build_stage_snapshot(git, cfg.internal_main_branch, ignore_patterns, frozenset({CONFIG_FILE}))
+        snapshot, lfs_count = build_stage_snapshot(
+            git, cfg.internal_main_branch, ignore_patterns, frozenset({CONFIG_FILE})
+        )
 
         unchanged_ref = snapshot_unchanged_ref(cfg, git, snapshot)
         if unchanged_ref is not None:
@@ -233,6 +238,9 @@ def stage(self, *, dry_run: bool = False, force: bool = False, no_pr: bool = Fal
         else:
             logger.info("Staging changes into %s", cfg.internal_approved_branch)
 
+        if lfs_count:
+            logger.info("Snapshot includes %d LFS-tracked %s", lfs_count, "file" if lfs_count == 1 else "files")
+
         if dry_run:
             logger.info("[dry-run] Would commit on %s", cfg.internal_stage_branch)
             logger.info("[dry-run] Would push %s to origin/%s", cfg.internal_stage_branch, cfg.internal_stage_branch)
@@ -383,6 +391,8 @@ def publish(self, *, dry_run: bool = False, force: bool = False, no_pr: bool = F
             )
             return
 
+        git.lfs_fetch("origin", origin_preview_ref)
+
         def _publish_work() -> bool:
             existing = git.ls_tree("HEAD")
             for path in existing:
@@ -518,7 +528,6 @@ def _absorb_startup(self) -> AbsorbResult:
         logger.debug("Starting absorb startup")
         self._require_on_main()
         self.git.fetch(self.cfg.public_remote)
-        self.git.lfs_fetch(self.cfg.public_remote, self.cfg.public_main_branch)
         self._prune_internal_pr_branches()
         self._prune_public_publish_branch()
         return check_absorb(self.cfg, self.git)
@@ -535,7 +544,6 @@ def _publish_startup(self) -> None:
         git, cfg = self.git, self.cfg
         git.ensure_clean_worktree()
         git.fetch("origin")
-        git.lfs_fetch("origin", cfg.internal_approved_branch)
         git.fetch(cfg.public_remote)
         self._prune_public_publish_branch()
 

pubgate/git.py

@@ -440,14 +440,22 @@ def lfs_fetch(self, remote: str, ref: str) -> None:
         if not self.is_lfs_available():
             return
         logger.info("Fetching LFS objects from %s for %s", remote, ref)
-        result = self._run("lfs", "fetch", remote, ref, check=False, timeout=_TIMEOUT_NETWORK)
+        try:
+            result = self._run("lfs", "fetch", remote, ref, check=False, timeout=_TIMEOUT_NETWORK)
+        except GitError as exc:
+            logger.warning("LFS fetch failed: %s", exc)
+            return
         if result.returncode != 0:
             logger.warning("LFS fetch failed (exit %d): %s", result.returncode, result.stderr.strip())
 
     def lfs_push(self, remote: str, branch: str) -> None:
         if not self.is_lfs_available():
             return
         logger.info("Pushing LFS objects to %s for %s", remote, branch)
-        result = self._run("lfs", "push", remote, branch, check=False, timeout=_TIMEOUT_NETWORK)
+        try:
+            result = self._run("lfs", "push", remote, branch, check=False, timeout=_TIMEOUT_NETWORK)
+        except GitError as exc:
+            logger.warning("LFS push failed: %s", exc)
+            return
         if result.returncode != 0:
             logger.warning("LFS push failed (exit %d): %s", result.returncode, result.stderr.strip())

pubgate/stage_snapshot.py

@@ -15,7 +15,7 @@ def build_stage_snapshot(
     ref: str,
     ignore_patterns: list[str],
     excluded: frozenset[str],
-) -> dict[str, str | bytes]:
+) -> tuple[dict[str, str | bytes], int]:
     all_files = git.ls_tree(ref)
     snapshot: dict[str, str | bytes] = {}
     lfs_files: list[str] = []
@@ -47,12 +47,10 @@ def build_stage_snapshot(
                 raise PubGateError(f"Error: {exc}") from exc
             snapshot[path] = content
 
-    if lfs_files:
-        logger.info("Snapshot includes %d LFS-tracked %s", len(lfs_files), "file" if len(lfs_files) == 1 else "files")
-        for path in lfs_files:
-            logger.debug("  LFS: %s", path)
+    for path in lfs_files:
+        logger.debug("  LFS: %s", path)
     logger.debug("Snapshot contains %d files", len(snapshot))
-    return snapshot
+    return snapshot, len(lfs_files)
 
 
 def snapshot_unchanged_ref(

tests/test_lfs.py

@@ -1,9 +1,11 @@
 import logging
+from unittest.mock import patch
 
 import pytest
 from conftest import Topology
 
 from pubgate.config import DEFAULT_IGNORE_PATTERNS
+from pubgate.errors import GitError
 from pubgate.filtering import is_ignored
 from pubgate.git import is_lfs_pointer
 from pubgate.stage_snapshot import build_stage_snapshot, snapshot_unchanged_ref
@@ -91,7 +93,7 @@ def test_lfs_pointer_detected_as_binary(self, topo: Topology):
 class TestStageSnapshotLfs:
     def test_lfs_pointer_passes_through_without_scrub(self, topo: Topology):
         topo.commit_internal({"data.bin": SAMPLE_LFS_POINTER_STR})
-        snapshot = build_stage_snapshot(
+        snapshot, _ = build_stage_snapshot(
             topo.work_dir.git,
             "HEAD",
             ignore_patterns=[],
@@ -104,7 +106,7 @@ def test_lfs_pointer_not_scrubbed_for_internal_markers(self, topo: Topology):
         # Even though the pointer text is valid UTF-8, it should not be
         # passed through scrub_internal_blocks
         topo.commit_internal({"model.bin": SAMPLE_LFS_POINTER_STR})
-        snapshot = build_stage_snapshot(
+        snapshot, _ = build_stage_snapshot(
             topo.work_dir.git,
             "HEAD",
             ignore_patterns=[],
@@ -125,7 +127,7 @@ def test_gitattributes_not_matched_by_default_patterns(self):
 
     def test_gitattributes_included_in_snapshot(self, topo: Topology):
         topo.commit_internal({".gitattributes": "*.bin filter=lfs diff=lfs merge=lfs -text\n"})
-        snapshot = build_stage_snapshot(
+        snapshot, _ = build_stage_snapshot(
             topo.work_dir.git,
             "HEAD",
             ignore_patterns=list(DEFAULT_IGNORE_PATTERNS),
@@ -186,7 +188,7 @@ def test_identical_lfs_pointer_detected_as_unchanged(self, topo: Topology):
         topo.work_dir.run("checkout", "main")
 
         # Build the same snapshot again (no changes)
-        snapshot = build_stage_snapshot(
+        snapshot, _ = build_stage_snapshot(
             topo.work_dir.git,
             topo.cfg.internal_main_branch,
             ignore_patterns=list(topo.cfg.ignore),
@@ -318,5 +320,74 @@ def test_lfs_pointer_with_gitattributes_in_publish(self, topo: Topology, monkeyp
         assert attrs is not None
         assert "filter=lfs" in attrs
 
-        pub_content = topo.external_contributor.read_file_at_ref("HEAD", "data.bin")
-        assert pub_content == SAMPLE_LFS_POINTER_STR
+
+# ---------------------------------------------------------------------------
+# LFS fetch/push failure handling
+# ---------------------------------------------------------------------------
+
+
+class TestLfsFetchFailure:
+    def test_nonzero_exit_logs_warning_and_continues(self, topo: Topology, caplog):
+        git = topo.work_dir.git
+        git._lfs_available = True  # force LFS as available
+        with caplog.at_level(logging.WARNING, logger="pubgate"):
+            # Fetch from a nonexistent remote ref — should warn, not raise
+            git.lfs_fetch("origin", "nonexistent-ref-that-will-fail")
+        assert "LFS fetch failed" in caplog.text
+
+    def test_timeout_logs_warning_and_continues(self, topo: Topology, caplog):
+        git = topo.work_dir.git
+        git._lfs_available = True
+
+        original_run = git._run
+
+        def _timeout_run(*args, **kwargs):
+            if args and args[0] == "lfs":
+                raise GitError(list(args), -1, "timed out after 300s")
+            return original_run(*args, **kwargs)
+
+        with patch.object(git, "_run", side_effect=_timeout_run):
+            with caplog.at_level(logging.WARNING, logger="pubgate"):
+                # Should not raise — timeout is caught and logged
+                git.lfs_fetch("origin", "main")
+        assert "LFS fetch failed" in caplog.text
+
+    def test_lfs_not_available_skips_silently(self, topo: Topology, caplog):
+        git = topo.work_dir.git
+        git._lfs_available = False
+        with caplog.at_level(logging.WARNING, logger="pubgate"):
+            git.lfs_fetch("origin", "main")
+        assert "LFS fetch failed" not in caplog.text
+
+
+class TestLfsPushFailure:
+    def test_timeout_logs_warning_and_continues(self, topo: Topology, caplog):
+        git = topo.work_dir.git
+        git._lfs_available = True
+
+        original_run = git._run
+
+        def _timeout_run(*args, **kwargs):
+            if args and args[0] == "lfs":
+                raise GitError(list(args), -1, "timed out after 300s")
+            return original_run(*args, **kwargs)
+
+        with patch.object(git, "_run", side_effect=_timeout_run):
+            with caplog.at_level(logging.WARNING, logger="pubgate"):
+                git.lfs_push("origin", "main")
+        assert "LFS push failed" in caplog.text
+
+    def test_nonzero_exit_logs_warning_and_continues(self, topo: Topology, caplog):
+        git = topo.work_dir.git
+        git._lfs_available = True
+        with caplog.at_level(logging.WARNING, logger="pubgate"):
+            # Push to a nonexistent remote — should warn, not raise
+            git.lfs_push("nonexistent-remote", "main")
+        assert "LFS push failed" in caplog.text
+
+    def test_lfs_not_available_skips_silently(self, topo: Topology, caplog):
+        git = topo.work_dir.git
+        git._lfs_available = False
+        with caplog.at_level(logging.WARNING, logger="pubgate"):
+            git.lfs_push("origin", "main")
+        assert "LFS push failed" not in caplog.text