fix(backup): fixed wiki backup, hardened security and reliability

Fixed wiki backup that never worked due to BackupResult dataclass
being incorrectly unpacked as tuple. Added PAT credential masking
in all log outputs and alert messages to prevent token leakage.
Elevated error logging from debug to error level for backup failures.
Added GitHub API retry logic with exponential backoff, proactive
rate limit handling, git clone timeout (1h), atomic state file writes,
and S3 bundle upload verification. Replaced GitPython with subprocess
for mirror clones, removing the dependency entirely. Pinned APScheduler
alpha version to prevent unexpected breaks.

Author: karlspace

src/backup/git_operations.py

@@ -6,18 +6,26 @@
 """
 
 import os
+import re
 import shutil
 import subprocess
 import tarfile
 from dataclasses import dataclass
 from pathlib import Path
 from typing import Optional
 
-from git import Repo, GitCommandError
-
 from ui.console import backup_logger
 
 
+def mask_credentials(text: str) -> str:
+    """Mask embedded credentials in URLs within a string.
+
+    Replaces patterns like https://TOKEN@github.com with https://***@github.com
+    to prevent leaking PATs in logs, alerts, or error messages.
+    """
+    return re.sub(r"https://[^@]+@", "https://***@", str(text))
+
+
 @dataclass
 class BackupResult:
     """Result of a repository backup operation."""
@@ -58,7 +66,8 @@ def mirror_clone(self, repo_url: str, repo_name: str) -> Path:
             Path to the cloned mirror repository.
 
         Raises:
-            GitCommandError: If cloning fails.
+            subprocess.CalledProcessError: If cloning fails.
+            subprocess.TimeoutExpired: If cloning exceeds timeout.
         """
         mirror_path = self.work_dir / f"{repo_name}.git"
 
@@ -68,14 +77,21 @@ def mirror_clone(self, repo_url: str, repo_name: str) -> Path:
 
         backup_logger.debug(f"Cloning {repo_name} as mirror...")
 
-        # Clone - let caller handle logging for failures
-        Repo.clone_from(
-            repo_url,
-            str(mirror_path),
-            mirror=True,
-            env={"GIT_TERMINAL_PROMPT": "0"}
+        result = subprocess.run(
+            ["git", "clone", "--mirror", repo_url, str(mirror_path)],
+            capture_output=True,
+            text=True,
+            env={**os.environ, "GIT_TERMINAL_PROMPT": "0"},
+            timeout=3600,  # 1 hour timeout
         )
 
+        if result.returncode != 0:
+            # Mask credentials in error output before raising
+            safe_stderr = mask_credentials(result.stderr)
+            raise subprocess.CalledProcessError(
+                result.returncode, "git clone --mirror", output=result.stdout, stderr=safe_stderr
+            )
+
         return mirror_path
 
     def is_empty_repo(self, mirror_path: Path) -> bool:
@@ -325,8 +341,3 @@ def cleanup(self) -> None:
                 item.unlink()
             elif item.is_dir():
                 shutil.rmtree(item)
-
-
-class WikiBackupError(Exception):
-    """Exception raised when wiki backup fails."""
-    pass

src/backup/github_client.py

@@ -8,15 +8,17 @@
 - Unauthenticated: Without GITHUB_PAT - public repos only, 60 requests/hour
 """
 
+import time
 from dataclasses import dataclass
-from datetime import datetime
+from datetime import datetime, timezone
 from typing import Generator, Optional, Union
 
-from github import Github, GithubException
+from github import Github, GithubException, RateLimitExceededException
 from github.Repository import Repository
 from github.Organization import Organization
 from github.AuthenticatedUser import AuthenticatedUser
 from github.NamedUser import NamedUser
+from urllib3.util.retry import Retry
 
 from config import Settings
 from ui.console import backup_logger
@@ -71,12 +73,15 @@ def __init__(self, settings: Settings):
         self.settings = settings
         self._authenticated = settings.is_authenticated
 
+        # Retry on transient server errors (502, 503, 504)
+        retry = Retry(total=3, backoff_factor=1, status_forcelist=[500, 502, 503, 504])
+
         if self._authenticated:
             # Use per_page=100 for better performance with large orgs
-            self.gh = Github(settings.github_pat, per_page=100)
+            self.gh = Github(settings.github_pat, per_page=100, retry=retry)
             backup_logger.debug("GitHub client initialized with authentication (5000 req/hour)")
         else:
-            self.gh = Github(per_page=100)  # Unauthenticated
+            self.gh = Github(per_page=100, retry=retry)  # Unauthenticated
             backup_logger.debug(
                 "GitHub client initialized WITHOUT authentication. "
                 "Only public repositories accessible (60 req/hour rate limit). "
@@ -110,6 +115,27 @@ def get_rate_limit_info(self) -> dict:
             "reset": rate.core.reset.isoformat() if rate.core.reset else None,
         }
 
+    def wait_for_rate_limit(self, min_remaining: int = 100) -> None:
+        """Check rate limit and wait if near exhaustion.
+
+        Args:
+            min_remaining: Minimum remaining requests before waiting.
+        """
+        try:
+            rate = self.gh.get_rate_limit()
+            remaining = rate.core.remaining
+            if remaining < min_remaining:
+                reset_time = rate.core.reset
+                wait_seconds = (reset_time - datetime.now(timezone.utc)).total_seconds() + 5
+                if wait_seconds > 0:
+                    backup_logger.warning(
+                        f"Rate limit low ({remaining}/{rate.core.limit} remaining), "
+                        f"waiting {wait_seconds:.0f}s until reset"
+                    )
+                    time.sleep(min(wait_seconds, 3600))  # Cap at 1 hour
+        except GithubException:
+            pass  # Don't fail backup over rate limit check itself
+
     def _resolve_owner(self) -> OwnerType:
         """Resolve the owner name to an Organization or User object.
 

src/backup/metadata_exporter.py

@@ -82,12 +82,15 @@ def export_issues(self, repo: Repository, output_path: Path) -> list[dict]:
 
         issues = []
         for issue in repo.get_issues(state="all"):
-            # Skip pull requests (they appear in issues API too)
-            if issue.pull_request:
-                continue
+            try:
+                # Skip pull requests (they appear in issues API too)
+                if issue.pull_request:
+                    continue
 
-            issue_data = self._issue_to_dict(issue)
-            issues.append(issue_data)
+                issue_data = self._issue_to_dict(issue)
+                issues.append(issue_data)
+            except GithubException as e:
+                backup_logger.warning(f"Failed to export issue #{issue.number} for {repo.name}: {e}")
 
         self._write_json(issues, output_path)
         return issues
@@ -106,8 +109,11 @@ def export_pull_requests(self, repo: Repository, output_path: Path) -> list[dict
 
         prs = []
         for pr in repo.get_pulls(state="all"):
-            pr_data = self._pr_to_dict(pr)
-            prs.append(pr_data)
+            try:
+                pr_data = self._pr_to_dict(pr)
+                prs.append(pr_data)
+            except GithubException as e:
+                backup_logger.warning(f"Failed to export PR #{pr.number} for {repo.name}: {e}")
 
         self._write_json(prs, output_path)
         return prs
@@ -126,8 +132,11 @@ def export_releases(self, repo: Repository, output_path: Path) -> list[dict]:
 
         releases = []
         for release in repo.get_releases():
-            release_data = self._release_to_dict(release)
-            releases.append(release_data)
+            try:
+                release_data = self._release_to_dict(release)
+                releases.append(release_data)
+            except GithubException as e:
+                backup_logger.warning(f"Failed to export release {release.tag_name} for {repo.name}: {e}")
 
         self._write_json(releases, output_path)
         return releases

src/backup/wiki_backup.py

@@ -4,13 +4,12 @@
 Handles backup of repository wikis.
 """
 
+import subprocess
 from pathlib import Path
 from typing import Optional
 
-from git import GitCommandError
-
 from ui.console import backup_logger
-from .git_operations import GitBackup
+from .git_operations import GitBackup, mask_credentials
 
 
 class WikiBackup:
@@ -45,21 +44,24 @@ def backup_wiki(
         wiki_name = f"{repo_name}.wiki"
 
         try:
-            bundle_path, bundle_size = self.git_backup.clone_and_bundle(
-                wiki_url, wiki_name
-            )
+            result = self.git_backup.clone_and_bundle(wiki_url, wiki_name)
+
+            if result.is_empty or result.bundle_path is None:
+                backup_logger.debug(f"Wiki is empty for {repo_name}")
+                return None, None
+
             backup_logger.info(f"Wiki backup created for {repo_name}")
-            return bundle_path, bundle_size
+            return result.bundle_path, result.bundle_size
 
-        except GitCommandError as e:
+        except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e:
             # Wiki might be enabled but empty, or access denied
-            error_msg = str(e).lower()
+            error_msg = mask_credentials(e).lower()
             if "repository not found" in error_msg or "not exist" in error_msg:
                 backup_logger.debug(f"Wiki not available for {repo_name} (empty or disabled)")
             else:
-                backup_logger.warning(f"Failed to backup wiki for {repo_name}: {e}")
+                backup_logger.warning(f"Failed to backup wiki for {repo_name}: {error_msg}")
             return None, None
 
         except Exception as e:
-            backup_logger.warning(f"Unexpected error backing up wiki for {repo_name}: {e}")
+            backup_logger.warning(f"Unexpected error backing up wiki for {repo_name}: {mask_credentials(e)}")
             return None, None

src/main.py

@@ -19,7 +19,7 @@
 from sync_state_manager import SyncStateManager
 from alerting.manager import AlertManager
 from backup.github_client import GitHubBackupClient
-from backup.git_operations import GitBackup, BackupResult
+from backup.git_operations import GitBackup, BackupResult, mask_credentials
 from backup.metadata_exporter import MetadataExporter
 from backup.wiki_backup import WikiBackup
 from storage.s3_client import S3Storage
@@ -238,6 +238,10 @@ def run_backup(settings: Settings) -> bool:
                             stats["lfs_repos"] = stats.get("lfs_repos", 0) + 1
                             s3_storage.upload_file(backup_result.lfs_path, backup_id, repo_name)
 
+                    # Check rate limit before metadata-heavy operations
+                    if settings.backup_include_metadata:
+                        gh_client.wait_for_rate_limit()
+
                     # Backup metadata (use underlying repo object)
                     if settings.backup_include_metadata:
                         meta_counts = metadata_exporter.export_all(repo_info.repo)
@@ -275,10 +279,11 @@ def run_backup(settings: Settings) -> bool:
                     )
 
                 except Exception as e:
-                    backup_logger.debug(f"Failed to backup {repo_name}: {e}")
-                    repo_stats["error"] = str(e)
+                    safe_error = mask_credentials(e)
+                    backup_logger.error(f"Failed to backup {repo_name}: {safe_error}")
+                    repo_stats["error"] = safe_error
                     stats["errors"] += 1
-                    error_messages.append(f"{repo_name}: {e}")
+                    error_messages.append(f"{repo_name}: {safe_error}")
 
                 # Print status for this repo
                 print_repo_status(

src/requirements.txt

@@ -7,11 +7,6 @@
 # ───────────────────────────────────────────────────────────────────────────────
 PyGithub>=2.8.0
 
-# ───────────────────────────────────────────────────────────────────────────────
-# Git Operations
-# ───────────────────────────────────────────────────────────────────────────────
-GitPython>=3.1.46
-
 # ───────────────────────────────────────────────────────────────────────────────
 # S3/MinIO Storage
 # ───────────────────────────────────────────────────────────────────────────────
@@ -20,7 +15,7 @@ boto3>=1.42.0
 # ───────────────────────────────────────────────────────────────────────────────
 # Scheduler
 # ───────────────────────────────────────────────────────────────────────────────
-APScheduler>=4.0.0a6
+APScheduler==4.0.0a6
 
 # ───────────────────────────────────────────────────────────────────────────────
 # CLI & Console UI

src/scheduler.py

@@ -44,7 +44,7 @@ def _run_backup_with_state(self) -> None:
                 self.state_manager.update_sync_time()
                 backup_logger.debug("Sync state updated after successful backup")
         except Exception as e:
-            backup_logger.debug(f"Backup execution failed: {e}")
+            backup_logger.error(f"Backup execution failed: {e}")
             raise
 
     def _job_listener(self, event: Event) -> None:

src/storage/s3_client.py

@@ -184,6 +184,18 @@ def upload_file(self, local_path: Path, backup_id: str, repo_name: str) -> str:
 
         try:
             self.uploader.upload_file(local_path, key)
+
+            # Verify critical uploads (bundles) arrived with correct size
+            if local_path.suffix == ".bundle":
+                response = self.s3.head_object(Bucket=self.bucket, Key=key)
+                remote_size = response["ContentLength"]
+                local_size = local_path.stat().st_size
+                if remote_size != local_size:
+                    raise RuntimeError(
+                        f"Upload verification failed for {key}: "
+                        f"local={local_size}, remote={remote_size}"
+                    )
+
             return key
         except ClientError as e:
             backup_logger.error(f"Failed to upload {local_path}: {e}")

src/sync_state_manager.py

@@ -9,6 +9,8 @@
 """
 
 import json
+import os
+import tempfile
 from datetime import datetime, timedelta
 from pathlib import Path
 from typing import Optional, TYPE_CHECKING
@@ -152,15 +154,28 @@ def _load_state(self) -> dict:
             return self._state
 
     def _save_state(self) -> None:
-        """Save state to file and sync to S3."""
+        """Save state to file atomically and sync to S3.
+
+        Uses write-to-temp-then-rename pattern to prevent corruption
+        if the process crashes mid-write.
+        """
         if self._state is None:
             return
 
         self._state["updated_at"] = datetime.now().isoformat()
 
         try:
-            with open(self.state_file, "w") as f:
-                json.dump(self._state, f, indent=2)
+            # Atomic write: temp file + rename prevents corruption on crash
+            fd, tmp_path = tempfile.mkstemp(
+                dir=str(self.state_file.parent), suffix=".tmp"
+            )
+            try:
+                with os.fdopen(fd, "w") as f:
+                    json.dump(self._state, f, indent=2)
+                os.replace(tmp_path, str(self.state_file))
+            except Exception:
+                os.unlink(tmp_path)
+                raise
             # Sync to S3 for persistence
             self._sync_state_to_s3()
         except IOError as e: