Browse Source

Fix SpoolBuddy update always pulling main instead of current branch

  detect_current_branch() was reading .git/HEAD from settings.base_dir,
  which points at the data volume (DATA_DIR=/app/data in Docker) and
  never contains .git. The repo is at /app, so the lookup always failed
  and the code fell through to the GIT_BRANCH env-var → "main" fallback.
  The SpoolBuddy device was therefore checking out `main` regardless of
  which branch Bambuddy itself was running.

  The old subprocess-based implementation had the same bug but it was
  masked: the stock Docker image has no `git` binary, so `git rev-parse`
  raised FileNotFoundError, the except clause swallowed it, and the
  fallback kicked in. Swapping to filesystem reads exposed the wrong
  lookup path.

  Add a module-level _APP_DIR constant (parents[3] of the module file,
  same depth as config.py uses for its own _app_dir) and read `.git/HEAD`
  from there. A regression test plants a decoy .git in the data dir and
  asserts we still pick up the real one from the app root.

  Per your NO GIT WRITES rule, nothing is staged or committed.
maziggy 1 month ago
parent
commit
7e7372c469

File diff suppressed because it is too large
+ 0 - 1
CHANGELOG.md


+ 22 - 10
backend/app/services/spoolbuddy_ssh.py

@@ -28,6 +28,13 @@ logger = logging.getLogger(__name__)
 SSH_USER = "spoolbuddy"
 DEFAULT_INSTALL_PATH = "/opt/bambuddy"
 
+# Project root — where the `.git` directory lives for native installs and for
+# Docker containers that bind-mount the repo. This is intentionally distinct
+# from `settings.base_dir`, which points at the persistent *data* directory
+# (e.g. `DATA_DIR=/app/data` in Docker) and therefore never contains `.git`.
+# `backend/app/services/spoolbuddy_ssh.py` → parents[3] = project root.
+_APP_DIR = Path(__file__).resolve().parents[3]
+
 # Note for Docker: asyncssh.connect() internally calls getpass.getuser() to
 # resolve the *local* username for ~/.ssh/config host matching. Under an
 # arbitrary PUID with no /etc/passwd entry this would raise OSError. The
@@ -93,22 +100,27 @@ async def get_public_key() -> str:
 def detect_current_branch() -> str:
     """Detect the git branch Bambuddy is running on.
 
-    Reads `.git/HEAD` directly rather than shelling out to `git`. This keeps
-    the behaviour identical for native installs, bare Docker containers
-    (no .git — fall through to the env var), and Docker containers that
-    bind-mount the repo (.git is present, no `git` binary required, and no
-    `getpwuid()` call that could fail under an arbitrary PUID).
-
-    Fallback order: `.git/HEAD` → `GIT_BRANCH` env var → `"main"`.
+    Reads `.git/HEAD` directly from the application root (``_APP_DIR``) rather
+    than shelling out to `git`. The application root is deliberately distinct
+    from ``settings.base_dir``: in Docker, ``base_dir`` points at the data
+    volume (``/app/data``) which never contains ``.git``, while the repo is
+    bind-mounted (or COPYd) to ``/app``. This works for native installs,
+    bare Docker containers (no ``.git`` — fall through to the env var), and
+    Docker containers that bind-mount the repo (``.git`` is present, no
+    ``git`` binary required, and no ``getpwuid()`` call that could fail under
+    an arbitrary PUID).
+
+    Fallback order: ``.git/HEAD`` → ``GIT_BRANCH`` env var → ``"main"``.
     """
-    git_path = settings.base_dir / ".git"
+    git_path = _APP_DIR / ".git"
     try:
         if git_path.exists():
-            # Git worktrees use a file containing `gitdir: <path>` instead of a dir.
+            # Git worktrees use a file containing `gitdir: <path>` instead of
+            # a directory — follow the pointer.
             if git_path.is_file():
                 content = git_path.read_text(encoding="utf-8").strip()
                 if content.startswith("gitdir:"):
-                    git_path = (settings.base_dir / content.removeprefix("gitdir:").strip()).resolve()
+                    git_path = (_APP_DIR / content.removeprefix("gitdir:").strip()).resolve()
 
             head_file = git_path / "HEAD"
             if head_file.is_file():

+ 36 - 11
backend/tests/unit/services/test_spoolbuddy_ssh.py

@@ -115,17 +115,16 @@ async def test_get_public_key(tmp_path):
 
 
 def test_detect_branch_from_git_head(tmp_path):
-    """Read branch directly from .git/HEAD — no subprocess."""
+    """Read branch directly from .git/HEAD in the application root — no subprocess."""
     git_dir = tmp_path / ".git"
     git_dir.mkdir()
     (git_dir / "HEAD").write_text("ref: refs/heads/dev\n")
 
     with (
-        patch("backend.app.services.spoolbuddy_ssh.settings") as mock_settings,
+        patch("backend.app.services.spoolbuddy_ssh._APP_DIR", tmp_path),
         patch("asyncio.create_subprocess_exec") as mock_exec,
         patch("subprocess.run") as mock_run,
     ):
-        mock_settings.base_dir = tmp_path
         assert detect_current_branch() == "dev"
         # Regression guard: must not shell out (fails with getpwuid under
         # arbitrary Docker PUIDs if ever reintroduced).
@@ -133,6 +132,36 @@ def test_detect_branch_from_git_head(tmp_path):
         mock_run.assert_not_called()
 
 
+def test_detect_branch_uses_app_dir_not_data_dir(tmp_path):
+    """Branch detection must look in the application root, not the data dir.
+
+    Regression guard for the Docker bug where `.git` was being looked up in
+    `settings.base_dir` (which is `DATA_DIR=/app/data` in Docker), so it was
+    never found and the fallback always returned "main" — even when the user
+    was on a feature branch bind-mounted at `/app`.
+    """
+    app_dir = tmp_path / "app"
+    data_dir = tmp_path / "app" / "data"
+    app_dir.mkdir()
+    data_dir.mkdir()
+
+    # Real .git lives at the application root (bind-mount style).
+    (app_dir / ".git").mkdir()
+    (app_dir / ".git" / "HEAD").write_text("ref: refs/heads/dev\n")
+
+    # Decoy .git in the data dir — if the code ever regresses to reading
+    # from settings.base_dir, this would be returned instead.
+    (data_dir / ".git").mkdir()
+    (data_dir / ".git" / "HEAD").write_text("ref: refs/heads/wrong-branch\n")
+
+    with (
+        patch("backend.app.services.spoolbuddy_ssh._APP_DIR", app_dir),
+        patch("backend.app.services.spoolbuddy_ssh.settings") as mock_settings,
+    ):
+        mock_settings.base_dir = data_dir
+        assert detect_current_branch() == "dev"
+
+
 def test_detect_branch_worktree_gitdir_file(tmp_path):
     """Git worktrees store a `gitdir:` pointer instead of a dir — follow it."""
     real_git_dir = tmp_path / "real-git"
@@ -140,8 +169,7 @@ def test_detect_branch_worktree_gitdir_file(tmp_path):
     (real_git_dir / "HEAD").write_text("ref: refs/heads/feature-x\n")
     (tmp_path / ".git").write_text(f"gitdir: {real_git_dir}\n")
 
-    with patch("backend.app.services.spoolbuddy_ssh.settings") as mock_settings:
-        mock_settings.base_dir = tmp_path
+    with patch("backend.app.services.spoolbuddy_ssh._APP_DIR", tmp_path):
         assert detect_current_branch() == "feature-x"
 
 
@@ -152,28 +180,25 @@ def test_detect_branch_detached_head_falls_back(tmp_path):
     (git_dir / "HEAD").write_text("deadbeef1234\n")
 
     with (
-        patch("backend.app.services.spoolbuddy_ssh.settings") as mock_settings,
+        patch("backend.app.services.spoolbuddy_ssh._APP_DIR", tmp_path),
         patch.dict(os.environ, {"GIT_BRANCH": "release"}),
     ):
-        mock_settings.base_dir = tmp_path
         assert detect_current_branch() == "release"
 
 
 def test_detect_branch_env_fallback(tmp_path):
     with (
-        patch("backend.app.services.spoolbuddy_ssh.settings") as mock_settings,
+        patch("backend.app.services.spoolbuddy_ssh._APP_DIR", tmp_path),
         patch.dict(os.environ, {"GIT_BRANCH": "staging"}),
     ):
-        mock_settings.base_dir = tmp_path
         assert detect_current_branch() == "staging"
 
 
 def test_detect_branch_default_main(tmp_path):
     with (
-        patch("backend.app.services.spoolbuddy_ssh.settings") as mock_settings,
+        patch("backend.app.services.spoolbuddy_ssh._APP_DIR", tmp_path),
         patch.dict(os.environ, {}, clear=True),
     ):
-        mock_settings.base_dir = tmp_path
         # Remove GIT_BRANCH if present
         os.environ.pop("GIT_BRANCH", None)
         assert detect_current_branch() == "main"

Some files were not shown because too many files changed in this diff