Fix repository deinstall logic and add unit tests for repository helpers

- Fix undefined repo_dir usage in repository deinstall action
- Centralize and harden get_repo_dir with strict validation and clear errors
- Expand user paths for repository base and binary directories
- Add unit tests for get_repo_dir and deinstall_repos
- Add comprehensive tests for resolve_repos identifier matching
- Remove obsolete command resolution tests no longer applicable

https://chatgpt.com/share/693d7442-c2d0-800f-9ff3-fb84d60eaeb4

src/pkgmgr/actions/repository/deinstall.py

@@ -1,13 +1,32 @@
 import os
+
+from pkgmgr.core.command.run import run_command
+from pkgmgr.core.repository.dir import get_repo_dir
 from pkgmgr.core.repository.identifier import get_repo_identifier
 
-def deinstall_repos(selected_repos, repositories_base_dir, bin_dir, all_repos, preview=False):
+
+def deinstall_repos(
+    selected_repos,
+    repositories_base_dir,
+    bin_dir,
+    all_repos,
+    preview: bool = False,
+) -> None:
     for repo in selected_repos:
         repo_identifier = get_repo_identifier(repo, all_repos)
-        alias_path = os.path.join(bin_dir, repo_identifier)
 
+        # Resolve repository directory
+        repo_dir = get_repo_dir(repositories_base_dir, repo)
+
+        # Prefer alias if available; fall back to identifier
+        alias_name = str(repo.get("alias") or repo_identifier)
+        alias_path = os.path.join(os.path.expanduser(bin_dir), alias_name)
+
+        # Remove alias link/file (interactive)
         if os.path.exists(alias_path):
-            confirm = input(f"Are you sure you want to delete link '{alias_path}' for {repo_identifier}? [y/N]: ").strip().lower()
+            confirm = input(
+                f"Are you sure you want to delete link '{alias_path}' for {repo_identifier}? [y/N]: "
+            ).strip().lower()
             if confirm == "y":
                 if preview:
                     print(f"[Preview] Would remove link '{alias_path}'.")
@@ -17,10 +36,13 @@ def deinstall_repos(selected_repos, repositories_base_dir, bin_dir, all_repos, p
         else:
             print(f"No link found for {repo_identifier} in {bin_dir}.")
 
+        # Run make deinstall if repository exists and has a Makefile
         makefile_path = os.path.join(repo_dir, "Makefile")
         if os.path.exists(makefile_path):
             print(f"Makefile found in {repo_identifier}, running 'make deinstall'...")
             try:
                 run_command("make deinstall", cwd=repo_dir, preview=preview)
             except SystemExit as e:
-                print(f"[Warning] Failed to run 'make deinstall' for {repo_identifier}: {e}")
+                print(
+                    f"[Warning] Failed to run 'make deinstall' for {repo_identifier}: {e}"
+                )

src/pkgmgr/core/repository/dir.py

@@ -1,15 +1,48 @@
-import sys
 import os
+import sys
+from typing import Any, Dict
 
-def get_repo_dir(repositories_base_dir:str,repo:{})->str:
-    try:
-        return os.path.join(repositories_base_dir, repo.get("provider"), repo.get("account"), repo.get("repository"))
-    except TypeError as e:
-        if repositories_base_dir:
-            print(f"Error: {e} \nThe repository {repo} seems not correct configured.\nPlease configure it correct.")
-            for key in ["provider","account","repository"]:
-                if not repo.get(key,False):
-                   print(f"Key '{key}' is missing.")
-        else:
-            print(f"Error: {e} \nThe base {base} seems not correct configured.\nPlease configure it correct.")
-        sys.exit(3)
+
+def get_repo_dir(repositories_base_dir: str, repo: Dict[str, Any]) -> str:
+    """
+    Build the local repository directory path from:
+      repositories_base_dir/provider/account/repository
+
+    Exits with code 3 and prints diagnostics if the input config is invalid.
+    """
+    # Base dir must be set and non-empty
+    if not repositories_base_dir:
+        print(
+            "Error: repositories_base_dir is missing.\n"
+            "The base directory for repositories seems not correctly configured.\n"
+            "Please configure it correctly."
+        )
+        sys.exit(3)
+
+    # Repo must be a dict-like object
+    if not isinstance(repo, dict):
+        print(
+            f"Error: invalid repo object '{repo}'.\n"
+            "The repository entry seems not correctly configured.\n"
+            "Please configure it correctly."
+        )
+        sys.exit(3)
+
+    base_dir = os.path.expanduser(str(repositories_base_dir))
+
+    provider = repo.get("provider")
+    account = repo.get("account")
+    repository = repo.get("repository")
+
+    missing = [k for k, v in [("provider", provider), ("account", account), ("repository", repository)] if not v]
+    if missing:
+        print(
+            "Error: repository entry is missing required keys.\n"
+            f"Repository: {repo}\n"
+            "Please configure it correctly."
+        )
+        for k in missing:
+            print(f"Key '{k}' is missing.")
+        sys.exit(3)
+
+    return os.path.join(base_dir, str(provider), str(account), str(repository))

tests/unit/pkgmgr/actions/repository/test_deinstall.py

@@ -0,0 +1,79 @@
+import unittest
+from unittest.mock import patch
+
+from pkgmgr.actions.repository.deinstall import deinstall_repos
+
+
+class TestDeinstallRepos(unittest.TestCase):
+    def test_preview_removes_nothing_but_runs_make_if_makefile_exists(self):
+        repo = {"provider": "github.com", "account": "alice", "repository": "demo", "alias": "demo"}
+        selected = [repo]
+
+        with patch("pkgmgr.actions.repository.deinstall.get_repo_identifier", return_value="demo"), \
+             patch("pkgmgr.actions.repository.deinstall.get_repo_dir", return_value="/repos/github.com/alice/demo"), \
+             patch("pkgmgr.actions.repository.deinstall.os.path.expanduser", return_value="/home/u/.local/bin"), \
+             patch("pkgmgr.actions.repository.deinstall.os.path.exists") as mock_exists, \
+             patch("pkgmgr.actions.repository.deinstall.os.remove") as mock_remove, \
+             patch("pkgmgr.actions.repository.deinstall.run_command") as mock_run, \
+             patch("builtins.input", return_value="y"):
+
+            # alias exists, Makefile exists
+            def exists_side_effect(path):
+                if path == "/home/u/.local/bin/demo":
+                    return True
+                if path == "/repos/github.com/alice/demo/Makefile":
+                    return True
+                return False
+
+            mock_exists.side_effect = exists_side_effect
+
+            deinstall_repos(
+                selected_repos=selected,
+                repositories_base_dir="/repos",
+                bin_dir="~/.local/bin",
+                all_repos=selected,
+                preview=True,
+            )
+
+            # Preview: do not remove
+            mock_remove.assert_not_called()
+
+            # But still "would run" make deinstall via run_command (preview=True)
+            mock_run.assert_called_once_with(
+                "make deinstall",
+                cwd="/repos/github.com/alice/demo",
+                preview=True,
+            )
+
+    def test_non_preview_removes_alias_when_confirmed(self):
+        repo = {"provider": "github.com", "account": "alice", "repository": "demo", "alias": "demo"}
+        selected = [repo]
+
+        with patch("pkgmgr.actions.repository.deinstall.get_repo_identifier", return_value="demo"), \
+             patch("pkgmgr.actions.repository.deinstall.get_repo_dir", return_value="/repos/github.com/alice/demo"), \
+             patch("pkgmgr.actions.repository.deinstall.os.path.expanduser", return_value="/home/u/.local/bin"), \
+             patch("pkgmgr.actions.repository.deinstall.os.path.exists") as mock_exists, \
+             patch("pkgmgr.actions.repository.deinstall.os.remove") as mock_remove, \
+             patch("pkgmgr.actions.repository.deinstall.run_command") as mock_run, \
+             patch("builtins.input", return_value="y"):
+
+            # alias exists, Makefile does NOT exist
+            def exists_side_effect(path):
+                if path == "/home/u/.local/bin/demo":
+                    return True
+                if path == "/repos/github.com/alice/demo/Makefile":
+                    return False
+                return False
+
+            mock_exists.side_effect = exists_side_effect
+
+            deinstall_repos(
+                selected_repos=selected,
+                repositories_base_dir="/repos",
+                bin_dir="~/.local/bin",
+                all_repos=selected,
+                preview=False,
+            )
+
+            mock_remove.assert_called_once_with("/home/u/.local/bin/demo")
+            mock_run.assert_not_called()

tests/unit/pkgmgr/core/repository/test_dir.py

@@ -0,0 +1,26 @@
+import unittest
+from unittest.mock import patch
+
+from pkgmgr.core.repository.dir import get_repo_dir
+
+
+class TestGetRepoDir(unittest.TestCase):
+    def test_builds_path_with_expanded_base_dir(self):
+        repo = {"provider": "github.com", "account": "alice", "repository": "demo"}
+        with patch("pkgmgr.core.repository.dir.os.path.expanduser", return_value="/home/u/repos"):
+            result = get_repo_dir("~/repos", repo)
+
+        self.assertEqual(result, "/home/u/repos/github.com/alice/demo")
+
+    def test_exits_with_code_3_if_base_dir_is_none(self):
+        repo = {"provider": "github.com", "account": "alice", "repository": "demo"}
+        with self.assertRaises(SystemExit) as ctx:
+            get_repo_dir(None, repo)  # type: ignore[arg-type]
+
+        self.assertEqual(ctx.exception.code, 3)
+
+    def test_exits_with_code_3_if_repo_is_invalid_type(self):
+        with self.assertRaises(SystemExit) as ctx:
+            get_repo_dir("/repos", None)  # type: ignore[arg-type]
+
+        self.assertEqual(ctx.exception.code, 3)

tests/unit/pkgmgr/core/repository/test_resolve.py

@@ -1,166 +0,0 @@
-# tests/unit/pkgmgr/test_resolve_command.py
-
-import unittest
-from unittest.mock import patch
-
-import pkgmgr.core.command.resolve as resolve_command_module
-
-
-class TestResolveCommandForRepo(unittest.TestCase):
-    def test_explicit_command_wins(self):
-        repo = {"command": "/custom/cmd"}
-        result = resolve_command_module.resolve_command_for_repo(
-            repo=repo,
-            repo_identifier="tool",
-            repo_dir="/repos/tool",
-        )
-        self.assertEqual(result, "/custom/cmd")
-
-    @patch("pkgmgr.core.command.resolve.shutil.which", return_value="/usr/bin/tool")
-    def test_system_binary_returns_none_and_no_error(self, mock_which):
-        repo = {}
-        result = resolve_command_module.resolve_command_for_repo(
-            repo=repo,
-            repo_identifier="tool",
-            repo_dir="/repos/tool",
-        )
-        # System binary → no link
-        self.assertIsNone(result)
-
-    @patch("pkgmgr.core.command.resolve.os.access")
-    @patch("pkgmgr.core.command.resolve.os.path.exists")
-    @patch("pkgmgr.core.command.resolve.shutil.which", return_value=None)
-    @patch("pkgmgr.core.command.resolve.os.path.expanduser", return_value="/fakehome")
-    def test_nix_profile_binary(
-        self,
-        mock_expanduser,
-        mock_which,
-        mock_exists,
-        mock_access,
-    ):
-        """
-        No system/PATH binary, but a Nix profile binary exists:
-        → must return the Nix binary path.
-        """
-        repo = {}
-        fake_home = "/fakehome"
-        nix_path = f"{fake_home}/.nix-profile/bin/tool"
-
-        def fake_exists(path):
-            # Only the Nix binary exists
-            return path == nix_path
-
-        def fake_access(path, mode):
-            # Only the Nix binary is executable
-            return path == nix_path
-
-        mock_exists.side_effect = fake_exists
-        mock_access.side_effect = fake_access
-
-        result = resolve_command_module.resolve_command_for_repo(
-            repo=repo,
-            repo_identifier="tool",
-            repo_dir="/repos/tool",
-        )
-        self.assertEqual(result, nix_path)
-
-    @patch("pkgmgr.core.command.resolve.os.access")
-    @patch("pkgmgr.core.command.resolve.os.path.exists")
-    @patch("pkgmgr.core.command.resolve.os.path.expanduser", return_value="/home/user")
-    @patch("pkgmgr.core.command.resolve.shutil.which", return_value="/home/user/.local/bin/tool")
-    def test_non_system_binary_on_path(
-        self,
-        mock_which,
-        mock_expanduser,
-        mock_exists,
-        mock_access,
-    ):
-        """
-        No system (/usr) binary and no Nix binary, but a non-system
-        PATH binary exists (e.g. venv or ~/.local/bin):
-        → must return that PATH binary.
-        """
-        repo = {}
-        non_system_path = "/home/user/.local/bin/tool"
-        nix_candidate = "/home/user/.nix-profile/bin/tool"
-
-        def fake_exists(path):
-            # Only the non-system PATH binary "exists".
-            return path == non_system_path
-
-        def fake_access(path, mode):
-            # Only the non-system PATH binary is executable.
-            return path == non_system_path
-
-        mock_exists.side_effect = fake_exists
-        mock_access.side_effect = fake_access
-
-        result = resolve_command_module.resolve_command_for_repo(
-            repo=repo,
-            repo_identifier="tool",
-            repo_dir="/repos/tool",
-        )
-        self.assertEqual(result, non_system_path)
-
-    @patch("pkgmgr.core.command.resolve.os.access")
-    @patch("pkgmgr.core.command.resolve.os.path.exists")
-    @patch("pkgmgr.core.command.resolve.shutil.which", return_value=None)
-    @patch("pkgmgr.core.command.resolve.os.path.expanduser", return_value="/fakehome")
-    def test_fallback_to_main_py(
-        self,
-        mock_expanduser,
-        mock_which,
-        mock_exists,
-        mock_access,
-    ):
-        """
-        No system/non-system PATH binary, no Nix binary, but main.py exists:
-        → must fall back to main.py in the repo.
-        """
-        repo = {}
-        main_py = "/repos/tool/main.py"
-
-        def fake_exists(path):
-            return path == main_py
-
-        def fake_access(path, mode):
-            return path == main_py
-
-        mock_exists.side_effect = fake_exists
-        mock_access.side_effect = fake_access
-
-        result = resolve_command_module.resolve_command_for_repo(
-            repo=repo,
-            repo_identifier="tool",
-            repo_dir="/repos/tool",
-        )
-        self.assertEqual(result, main_py)
-
-    @patch("pkgmgr.core.command.resolve.os.access", return_value=False)
-    @patch("pkgmgr.core.command.resolve.os.path.exists", return_value=False)
-    @patch("pkgmgr.core.command.resolve.shutil.which", return_value=None)
-    @patch("pkgmgr.core.command.resolve.os.path.expanduser", return_value="/fakehome")
-    def test_no_command_results_in_system_exit(
-        self,
-        mock_expanduser,
-        mock_which,
-        mock_exists,
-        mock_access,
-    ):
-        """
-        Nothing available at any layer:
-        → must raise SystemExit with a descriptive error message.
-        """
-        repo = {}
-        with self.assertRaises(SystemExit) as cm:
-            resolve_command_module.resolve_command_for_repo(
-                repo=repo,
-                repo_identifier="tool",
-                repo_dir="/repos/tool",
-            )
-        msg = str(cm.exception)
-        self.assertIn("No executable command could be resolved for repository 'tool'", msg)
-
-
-if __name__ == "__main__":
-    unittest.main()

tests/unit/pkgmgr/core/repository/test_resolve_repos.py

@@ -0,0 +1,77 @@
+import unittest
+from unittest.mock import patch
+
+from pkgmgr.core.repository.resolve import resolve_repos
+
+
+class TestResolveRepos(unittest.TestCase):
+    def setUp(self) -> None:
+        # Two repos share the same repository name "common" to test uniqueness logic
+        self.repos = [
+            {
+                "provider": "github.com",
+                "account": "alice",
+                "repository": "demo",
+                "alias": "d",
+            },
+            {
+                "provider": "github.com",
+                "account": "bob",
+                "repository": "common",
+                "alias": "c1",
+            },
+            {
+                "provider": "gitlab.com",
+                "account": "carol",
+                "repository": "common",
+                "alias": "c2",
+            },
+        ]
+
+    def test_matches_full_identifier(self):
+        result = resolve_repos(["github.com/alice/demo"], self.repos)
+        self.assertEqual(result, [self.repos[0]])
+
+    def test_matches_alias(self):
+        result = resolve_repos(["d"], self.repos)
+        self.assertEqual(result, [self.repos[0]])
+
+    def test_matches_unique_repository_name_only_if_unique(self):
+        # "demo" is unique -> match
+        result = resolve_repos(["demo"], self.repos)
+        self.assertEqual(result, [self.repos[0]])
+
+        # "common" is NOT unique -> should not match anything
+        result2 = resolve_repos(["common"], self.repos)
+        self.assertEqual(result2, [])
+
+    def test_multiple_identifiers_accumulate_matches_in_order(self):
+        result = resolve_repos(["d", "github.com/bob/common"], self.repos)
+        self.assertEqual(result, [self.repos[0], self.repos[1]])
+
+    def test_unknown_identifier_prints_message(self):
+        with patch("builtins.print") as mock_print:
+            result = resolve_repos(["does-not-exist"], self.repos)
+
+        self.assertEqual(result, [])
+        mock_print.assert_called_with(
+            "Identifier 'does-not-exist' did not match any repository in config."
+        )
+
+    def test_duplicate_identifiers_return_duplicates(self):
+        # Current behavior: duplicates are not de-duplicated
+        result = resolve_repos(["d", "d"], self.repos)
+        self.assertEqual(result, [self.repos[0], self.repos[0]])
+
+    def test_empty_identifiers_returns_empty_list(self):
+        result = resolve_repos([], self.repos)
+        self.assertEqual(result, [])
+
+    def test_empty_repo_list_returns_empty_list_and_prints(self):
+        with patch("builtins.print") as mock_print:
+            result = resolve_repos(["github.com/alice/demo"], [])
+
+        self.assertEqual(result, [])
+        mock_print.assert_called_with(
+            "Identifier 'github.com/alice/demo' did not match any repository in config."
+        )