fix: resolve the seven bugs pinned by the test audit (#67)

AppriseClient.py

@@ -11,19 +11,16 @@ def apprise_notify(req_obj, host, port, aurls, title, body):
 class AppriseClient:
     def __init__(self):
         self.config = ''
-        try:
-            if os.environ["DOCKER"]:
+        if os.environ.get("DOCKER"):
             self.host = os.environ["host"]
             self.port = os.environ["port"]
             self.aurls = os.environ["aurls"]
             self.title = os.environ["title"]
             self.body = os.environ["body"]
-            if os.environ["toml_path"]:
+        if os.environ.get("toml_path"):
             config_file_path=os.environ["toml_path"]
             with open(config_file_path, 'rb') as c:
                 self.config = load(c)
-        except:
-            KeyError
         if os.path.exists('./config.toml'):
             config_file_path = './config.toml'
             with open(config_file_path, 'rb') as c:

config.toml.example

@@ -50,6 +50,10 @@ aurls = 'mailto://user:pass@gmail.com'
 enable_dragnet = false
 dragnet_outfile = "./orphaned.csv"
 
+[telemetry]
+enable_telemetry = false
+telemetry_outfile = "./telemetry.csv"
+
 [ignored_categories]
 tech = "tech"
 books = "books"

qbit-maid.py

@@ -89,7 +89,7 @@ class Qbt:
         self.tracker_whitelist = self.config["ignored_domains"]
 
         #ignored_tags
-        self.ignored_tags = self.config["ignored_domains"]
+        self.ignored_tags = self.config["ignored_tags"]
 
         #healthcheck
         self.use_healthcheck = self.config["healthcheck"]["use_healthcheck"]
@@ -117,7 +117,7 @@ class Qbt:
             self.tl.info('Connected.')
         except qbittorrentapi.APIError as e:
             self.tl.exception(e)
-            self.po.send_message(e, title="qbit-maid API ERROR")
+            tor_notify_error(self, e)
         # Pulling all torrent data
         self.torrent_list = self.qbt_client.torrents_info()
         #Main process block

qlist.py

@@ -22,24 +22,22 @@ def build_tor_list(self):
                 self.ignored_counter += 1
                 continue
             if is_tag_blank(torrent['tags']):
+                tag = torrent_tag_for(
+                    torrent['tracker'],
+                    self.tracker_protected_tag,
+                    self.tracker_non_protected_tag,
+                    self.tracker_whitelist.values(),
+                )
+                if tag:
                     self.tl.debug(f'Tagging new torrent: ["{torrent["name"][0:20]}..."] {torrent["tracker"]}hash: {torrent["hash"]}')
-                if is_protected_tracker(torrent['tracker'], self.tracker_whitelist.values()):
-                    self.qbt_client.torrents_add_tags(self.tracker_protected_tag,torrent['hash'])
-                elif is_not_protected_tracker(torrent['tracker'], self.tracker_whitelist.values()):
-                    self.qbt_client.torrents_add_tags(self.tracker_non_protected_tag,torrent['hash'])
+                    self.qbt_client.torrents_add_tags(tag, torrent['hash'])
             if is_preme(torrent['seeding_time'], self.min_age):
                 self.preme_tor_counter += 1
                 self.tl.debug(f'Premature torrent: ["{torrent["name"][0:20]}..."] Seconds Seeded: [{torrent["seeding_time"]}] hash: {torrent["hash"]}')
                 continue
             elif is_protected_tracker(torrent['tracker'], self.tracker_whitelist.values()):
-                if is_tag_blank(torrent['tags']):
-                    self.qbt_client.torrents_add_tags(self.tracker_protected_tag,torrent['hash'])
-                    self.tl.debug(f'Tagging Protected torrent: ["{torrent["name"][0:20]}..."] {torrent["tracker"]}hash: {torrent["hash"]}')
                 self.tracker_list.append(torrent)
             elif is_not_protected_tracker(torrent['tracker'], self.tracker_whitelist.values()):
-                if is_tag_blank(torrent['tags']):
-                    self.qbt_client.torrents_add_tags(self.tracker_non_protected_tag,torrent['hash'])
-                    self.tl.debug(f'Tagging Non-protected torrent: ["{torrent["name"][0:20]}..."] {torrent["tracker"]}hash: {torrent["hash"]}')
                 self.tracker_list.append(torrent)
 
 
@@ -67,11 +65,26 @@ def is_not_protected_tracker(tracker, trackerlist):
     if tracker.split('/')[2] not in trackerlist:
         return True
 
+def torrent_tag_for(tracker, protected_tag, non_protected_tag, trackerlist):
+    """Return the tag a torrent should carry, or None for an unknown tracker."""
+    if is_protected_tracker(tracker, trackerlist):
+        return protected_tag
+    if is_not_protected_tracker(tracker, trackerlist):
+        return non_protected_tag
+    return None
+
 def is_tag_blank(tag):
     if tag == '':
         return True
 
 def is_ignored_tag(igtags, tortags):
-    for igt in igtags:
-        if igt in tortags:
-            return True
+    """True when the torrent carries one of the configured ignored tags.
+
+    qBittorrent returns tags as a comma-separated string, so the comparison is
+    per tag: a configured ``ipt`` must not match a torrent tagged
+    ``iptorrents``.
+    """
+    if not tortags:
+        return False
+    torrent_tags = {tag.strip() for tag in str(tortags).split(',') if tag.strip()}
+    return any(igt in torrent_tags for igt in igtags)

qlogging.py

@@ -77,9 +77,30 @@ def tor_notify_summary(self):
     except Exception as e:  # never let a notification failure abort a run
         logger.error(f"Failed to send pushover notification: {e}")
 
+def tor_notify_error(self, error, title="qbit-maid API ERROR"):
+    """Report an error through pushover when it is configured.
+
+    The vendored ``pushover`` module only exposes the ``Pushover`` class, so the
+    message has to go through the client built by :func:`tor_notify`.
+    """
+    logger = _logger(self)
+    logger.error(f"{title}: {error}")
+    if not getattr(self, "use_pushover", False):
+        return
+    client = getattr(self, "poc", None)
+    if client is None:
+        return
+    try:
+        client.message(self.po_key, str(error), title=title)
+    except Exception as exc:  # never let a notification failure abort a run
+        logger.error(f"Failed to send pushover error notification: {exc}")
+
 def list_first_tor(self, index=0):
     """Only lists the first torrent"""
     self.tl.debug('First torrent in the list:')
+    if not self.torrent_list:
+        self.tl.debug('No torrents found.')
+        return
     torrent = self.torrent_list[index]
     for k,v in torrent.items():
          self.tl.debug(f'{k}:  {v}')
@@ -337,6 +358,7 @@ def setup_logging(use_log: bool, log_path: str, log_level: str,
     # Make setup idempotent: clear any handlers from a previous call
     for handler in list(logger.handlers):
         logger.removeHandler(handler)
+        handler.close()
 
     if not use_log:
         logger.addHandler(logging.NullHandler())

test_apprise.py

@@ -143,8 +143,6 @@ class TestAppriseClientConfig(unittest.TestCase):
         return config_path
 
     def test_toml_path_is_read_when_docker_is_set(self):
-        # The DOCKER branch reads five env vars before toml_path is reached, so
-        # all of them must be present for the toml_path branch to run at all.
         os.environ.update({
             "DOCKER": "1",
             "host": "env-host",
@@ -158,15 +156,12 @@ class TestAppriseClientConfig(unittest.TestCase):
         self.assertEqual(self.captured["host"], "toml-host")
         self.assertEqual(self.captured["port"], 4321)
 
-    def test_toml_path_is_ignored_without_docker(self):
-        """KNOWN BUG: the bare ``except`` swallows the DOCKER KeyError and
-        skips the rest of the try block, so ``toml_path`` is never read unless
-        ``DOCKER`` is also set. With no ./config.toml either, the client then
-        crashes on the missing ``host`` attribute.
-        """
+    def test_toml_path_is_read_without_docker(self):
         os.environ["toml_path"] = self._write_elsewhere_config()
-        with self.assertRaises(AttributeError):
         AppriseClient()
+        self.assertEqual(self.captured["host"], "toml-host")
+        self.assertEqual(self.captured["port"], 4321)
+        self.assertEqual(self.captured["title"], "Toml Title")
 
 
 if __name__ == "__main__":

test_qbitmaid.py

@@ -113,13 +113,16 @@ class TestPredicates(unittest.TestCase):
         self.assertTrue(is_ignored_tag(["save"], "save,public,ipt"))
         self.assertFalse(is_ignored_tag(["save"], "public,ipt"))
 
-    def test_is_ignored_tag_matches_substrings_not_tags(self):
-        """KNOWN BUG: matching is substring-based, so a short tag matches any
-        longer tag that contains it. 'ipt' matches 'iptorrents' and 'save'
-        matches 'unsaved', which silently ignores unrelated torrents.
+    def test_is_ignored_tag_matches_whole_tags_only(self):
+        """A configured tag must match a whole tag, not a substring of one.
+
+        'ipt' must not match 'iptorrents' and 'save' must not match 'unsaved'.
         """
-        self.assertTrue(is_ignored_tag(["ipt"], "iptorrents"))
-        self.assertTrue(is_ignored_tag(["save"], "unsaved"))
+        self.assertFalse(is_ignored_tag(["ipt"], "iptorrents"))
+        self.assertFalse(is_ignored_tag(["save"], "unsaved"))
+        self.assertTrue(is_ignored_tag(["ipt"], "save,ipt"))
+        self.assertTrue(is_ignored_tag(["save"], " save , public"))
+        self.assertFalse(is_ignored_tag(["save"], ""))
 
 
 class TestSendPing(unittest.TestCase):

test_qbt.py

@@ -13,7 +13,13 @@ import types
 import unittest
 from unittest import mock
 
-from qbt_fakes import FakeAPIError, FakeQbtClient, StubPushover, torrent
+from qbt_fakes import (
+    FailingPushover,
+    FakeAPIError,
+    FakeQbtClient,
+    StubPushover,
+    torrent,
+)
 
 REPO = os.path.dirname(os.path.abspath(__file__))
 
@@ -200,13 +206,21 @@ class TestQbtPipeline(QbtTestCase):
         self.assertIsNotNone(qbt.et)
         self.assertGreaterEqual(qbt.et, qbt.st)
 
-    def test_empty_torrent_list_crashes(self):
-        """KNOWN BUG: list_first_tor() indexes torrent_list[0] unconditionally,
-        so a qBittorrent instance with zero torrents raises IndexError and the
-        whole run aborts before any summary is sent.
-        """
-        with self.assertRaises(IndexError):
-            self.build([])
+    def test_empty_torrent_list_is_handled(self):
+        """A qBittorrent instance with zero torrents must not abort the run."""
+        qbt, client = self.build([])
+        self.assertEqual(qbt.total_torrents, 0)
+        self.assertEqual(qbt.torrent_hash_delete_list, [])
+        self.assertEqual(client.deleted, [])
+
+    def test_empty_torrent_list_still_sends_a_summary(self):
+        stub = StubPushover()
+        with mock.patch.object(self.module.pushover, "Pushover",
+                               return_value=stub):
+            self.build([], use_pushover="true")
+        self.assertEqual(len(stub.calls), 1)
+        self.assertEqual(stub.calls[0]["kwargs"]["title"],
+                         "qbit-maid: nothing removed")
 
 
 class TestQbtConfigSources(QbtTestCase):
@@ -281,23 +295,32 @@ class TestQbtLoginFailure(QbtTestCase):
     def test_login_error_notifies_and_continues(self):
         client = FakeQbtClient(torrents=[torrent(tags="ipt")],
                                login_error=FakeAPIError("bad credentials"))
-        with mock.patch.object(self.module.pushover, "send_message",
-                               create=True) as send_message:
-            qbt, _ = self.build([torrent(tags="ipt")], client=client)
-        send_message.assert_called_once()
-        self.assertEqual(send_message.call_args.kwargs["title"],
+        stub = StubPushover()
+        with mock.patch.object(self.module.pushover, "Pushover",
+                               return_value=stub):
+            qbt, _ = self.build([torrent(tags="ipt")], client=client,
+                                use_pushover="true")
+        # The error notification goes out first, then the run summary.
+        self.assertEqual(len(stub.calls), 2)
+        self.assertEqual(stub.calls[0]["kwargs"]["title"],
                          "qbit-maid API ERROR")
+        self.assertIn("bad credentials", stub.calls[0]["body"])
         self.assertEqual(qbt.total_torrents, 1)
 
-    def test_login_error_crashes_without_a_send_message_helper(self):
-        """KNOWN BUG: the vendored pushover module has no module-level
-        ``send_message``, so the error path raises AttributeError instead of
-        notifying anyone.
-        """
+    def test_login_error_without_pushover_does_not_crash(self):
         client = FakeQbtClient(torrents=[torrent(tags="ipt")],
                                login_error=FakeAPIError("bad credentials"))
-        with self.assertRaises(AttributeError):
-            self.build([torrent(tags="ipt")], client=client)
+        qbt, _ = self.build([torrent(tags="ipt")], client=client)
+        self.assertEqual(qbt.total_torrents, 1)
+
+    def test_login_error_survives_a_failing_pushover(self):
+        client = FakeQbtClient(torrents=[torrent(tags="ipt")],
+                               login_error=FakeAPIError("bad credentials"))
+        with mock.patch.object(self.module.pushover, "Pushover",
+                               return_value=FailingPushover()):
+            qbt, _ = self.build([torrent(tags="ipt")], client=client,
+                                use_pushover="true")
+        self.assertEqual(qbt.total_torrents, 1)
 
 
 class TestMainGuard(QbtTestCase):
@@ -312,14 +335,15 @@ class TestMainGuard(QbtTestCase):
 
 
 class TestIgnoredTagsConfig(QbtTestCase):
-    def test_ignored_tags_reads_the_ignored_domains_section(self):
-        """KNOWN BUG: qbit-maid.py assigns ``ignored_tags`` from
-        ``config["ignored_domains"]``, so the ``[ignored_tags]`` section is
-        never read and tag-based ignoring silently does nothing.
-        """
+    def test_ignored_tags_reads_the_ignored_tags_section(self):
         qbt, _ = self.build([torrent(tags="ipt")])
-        self.assertEqual(qbt.ignored_tags, {"tracker-example": "tracker.example"})
-        self.assertNotIn("save", qbt.ignored_tags)
+        self.assertEqual(qbt.ignored_tags, {"save": "save"})
+        self.assertNotIn("tracker-example", qbt.ignored_tags)
+
+    def test_ignored_tag_from_config_is_skipped(self):
+        qbt, _ = self.build([torrent(tags="save", infohash="ignored-tag")])
+        self.assertEqual(qbt.ignored_counter, 1)
+        self.assertEqual(qbt.tracker_list, [])
 
 
 if __name__ == "__main__":

test_qlist.py

@@ -2,7 +2,28 @@
 import unittest
 
 from qbt_fakes import FakeQbt, torrent
-from qlist import build_tor_list
+from qlist import build_tor_list, torrent_tag_for
+
+
+class TestTorrentTagFor(unittest.TestCase):
+    def test_protected_tracker_gets_the_protected_tag(self):
+        self.assertEqual(
+            torrent_tag_for("https://tracker.example/a", "ipt", "public",
+                            ["tracker.example"]),
+            "ipt",
+        )
+
+    def test_unknown_tracker_gets_the_public_tag(self):
+        self.assertEqual(
+            torrent_tag_for("https://other.example/a", "ipt", "public",
+                            ["tracker.example"]),
+            "public",
+        )
+
+    def test_blank_tracker_gets_no_tag(self):
+        self.assertIsNone(
+            torrent_tag_for("", "ipt", "public", ["tracker.example"])
+        )
 
 
 class TestBuildTorList(unittest.TestCase):
@@ -36,6 +57,14 @@ class TestBuildTorList(unittest.TestCase):
         self.assertEqual(qbt.ignored_counter, 1)
         self.assertEqual(qbt.tracker_list, [])
 
+    def test_ignored_tag_does_not_match_a_longer_tag(self):
+        qbt = self.run_build(
+            [torrent(tags="iptorrents", tracker="https://other.example/a")],
+            ignored_tags={"ipt": "ipt"},
+        )
+        self.assertEqual(qbt.ignored_counter, 0)
+        self.assertEqual(len(qbt.tracker_list), 1)
+
     def test_blank_tag_on_protected_tracker_gets_protected_tag(self):
         qbt = self.run_build([torrent(tags="", infohash="h1")])
         self.assertIn(("ipt", "h1"), qbt.qbt_client.added_tags)
@@ -48,16 +77,10 @@ class TestBuildTorList(unittest.TestCase):
         self.assertIn(("public", "h2"), qbt.qbt_client.added_tags)
         self.assertEqual(len(qbt.tracker_list), 1)
 
-    def test_blank_tag_is_applied_twice_for_non_premature_torrents(self):
-        """KNOWN REDUNDANCY: the tagging block runs twice for the same torrent.
-
-        Lines 24-29 tag any blank-tag torrent; lines 34-43 tag it again inside
-        the tracker branch. qBittorrent treats the second call as a no-op, so
-        the outcome is correct but one API call per new torrent is wasted.
-        Pinned here so the duplication is visible rather than silent.
-        """
+    def test_blank_tag_is_applied_once_for_non_premature_torrents(self):
+        """A blank-tag torrent is tagged once, not once per code path."""
         qbt = self.run_build([torrent(tags="", infohash="h1")])
-        self.assertEqual(qbt.qbt_client.added_tags, [("ipt", "h1"), ("ipt", "h1")])
+        self.assertEqual(qbt.qbt_client.added_tags, [("ipt", "h1")])
 
     def test_premature_blank_tag_torrent_is_tagged_once(self):
         """Premature torrents are tagged by the first block, then skipped."""

test_qlogging_extra.py

@@ -5,10 +5,9 @@ import os
 import shutil
 import tempfile
 import unittest
-import warnings
 from collections import Counter
 
-from qbt_fakes import FakeQbt, StubPushover, torrent
+from qbt_fakes import FailingPushover, FakeQbt, StubPushover, torrent
 from qlogging import (
     LOGGER_NAME,
     debug_torrent_list,
@@ -24,6 +23,7 @@ from qlogging import (
     summary_runtime,
     summary_stats,
     tor_notify,
+    tor_notify_error,
     tor_notify_summary,
     tornotifytest,
     torrent_count,
@@ -76,6 +76,35 @@ class TestNotifySetup(unittest.TestCase):
         self.assertEqual(qbt.poc.calls[0]["kwargs"]["title"], "qbit-maid")
 
 
+class TestTorNotifyError(unittest.TestCase):
+    def test_sends_through_the_pushover_client(self):
+        qbt = FakeQbt(use_pushover=True)
+        tor_notify_error(qbt, RuntimeError("boom"))
+        self.assertEqual(len(qbt.poc.calls), 1)
+        self.assertEqual(qbt.poc.calls[0]["user"], "USERKEY")
+        self.assertIn("boom", qbt.poc.calls[0]["body"])
+        self.assertEqual(qbt.poc.calls[0]["kwargs"]["title"],
+                         "qbit-maid API ERROR")
+
+    def test_skips_pushover_when_disabled(self):
+        qbt = FakeQbt(use_pushover=False)
+        tor_notify_error(qbt, RuntimeError("boom"))
+        self.assertEqual(qbt.poc.calls, [])
+
+    def test_skips_pushover_when_no_client_was_built(self):
+        qbt = FakeQbt(use_pushover=True, poc=None)
+        tor_notify_error(qbt, RuntimeError("boom"))  # must not raise
+
+    def test_survives_a_failing_pushover(self):
+        qbt = FakeQbt(use_pushover=True, poc=FailingPushover())
+        tor_notify_error(qbt, RuntimeError("boom"))  # must not raise
+
+    def test_accepts_a_custom_title(self):
+        qbt = FakeQbt(use_pushover=True)
+        tor_notify_error(qbt, RuntimeError("boom"), title="Custom")
+        self.assertEqual(qbt.poc.calls[0]["kwargs"]["title"], "Custom")
+
+
 class TestCounters(unittest.TestCase):
     def test_process_counts_buckets_by_tag(self):
         qbt = FakeQbt(
@@ -117,6 +146,12 @@ class TestDebugHelpers(unittest.TestCase):
             list_first_tor(qbt, index=1)
         self.assertIn("One", "\n".join(captured.output))
 
+    def test_list_first_tor_handles_an_empty_list(self):
+        qbt = FakeQbt(torrent_list=[])
+        with self.assertLogs("qbit-maid-test", level="DEBUG") as captured:
+            list_first_tor(qbt)
+        self.assertIn("No torrents found", "\n".join(captured.output))
+
     def test_list_qbit_api_info_logs_versions(self):
         qbt = FakeQbt()
         with self.assertLogs("qbit-maid-test", level="DEBUG") as captured:
@@ -233,14 +268,13 @@ class TestSetupLogging(unittest.TestCase):
         self.assertEqual(logger.level, logging.INFO)
 
     def test_setup_is_idempotent(self):
-        setup_logging(True, self.log_path, "INFO")
-        # setup_logging drops the previous handlers without closing them, so a
-        # second call leaks the old file descriptor. Known issue; silenced here
-        # so the suite output stays readable.
-        with warnings.catch_warnings():
-            warnings.simplefilter("ignore", ResourceWarning)
+        first = setup_logging(True, self.log_path, "INFO")
+        previous_file_handler = first.handlers[0]
+        self.assertIsNotNone(previous_file_handler.stream)
         logger = setup_logging(True, self.log_path, "INFO")
         self.assertEqual(len(logger.handlers), 2)
+        # The replaced handler is closed, so its file descriptor is released.
+        self.assertIsNone(previous_file_handler.stream)
 
     def test_rotation_parameters_are_applied(self):
         logger = setup_logging(True, self.log_path, "INFO",